Page MenuHomePhabricator

[X86] Delay creating index register negations during address matching until after we know for sure the match will succeed
ClosedPublic

Authored by craig.topper on Apr 23 2019, 5:18 PM.

Details

Summary

If we're trying to match an LEA, its possible the LEA match will be deemed unprofitable. In which case the negation we created in matchAddress would be left dangling in the SelectionDAG. This could artificially increase use counts for other nodes in the DAG. Though I don't have an example of that. But it just seems like bad form to have dangling nodes in isel.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 23 2019, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2019, 5:18 PM
spatel added inline comments.Apr 24 2019, 11:32 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
267 ↗(On Diff #196357)

Can we deal with negation here rather than duplicating code in the callers?

craig.topper marked an inline comment as done.Apr 24 2019, 12:15 PM
craig.topper added inline comments.
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
267 ↗(On Diff #196357)

We're missing the address root we would need for the insertDAGNode calls, but I can add it.

craig.topper planned changes to this revision.Apr 24 2019, 2:39 PM

-Move into getAddressOperands.
-Emit the NEG machine directly as I'm not sure I trust accessing N at this point in the code to use insertDAGNode. N can be changed by CSEing and other DAG modifications that can occur during address matching.
-Add -pre-RA-sched=linearize to one test that would fail previously due to the dangling sub node getting left behind when LEA costing failed.

Is PR39452 related to this?

Is PR39452 related to this?

I don't think so. I was tracking other ways to trigger the assert from PR22614.

spatel accepted this revision.Wed, May 15, 8:36 AM

LGTM

llvm/test/CodeGen/X86/imul.ll
6 ↗(On Diff #199301)

Is it worth doing FileCheck on this run too? Either it's the same as the others or different in some way that is interesting?

This revision is now accepted and ready to land.Wed, May 15, 8:36 AM
This revision was automatically updated to reflect the committed changes.