This is an archive of the discontinued LLVM Phabricator instance.

DAGCombiner: Don't unnecessarily swap operands in ReassociateOps
ClosedPublic

Authored by arsenm on Feb 10 2015, 12:22 PM.

Details

Summary

In the case where op = add, y = base_ptr, and x = offset, this
transform:

(op y, (op x, c1)) -> (op (op x, y), c1)

breaks the canonical form of add by putting the base pointer in the
second operand and the offset in the first.

This fix is important for the R600 target, because for some address
spaces the base pointer and the offset are stored in separate register
classes. The old pattern caused the ISel code for matching addressing
modes to put the base pointer and offset in the wrong register classes,
which required no-trivial code transformations to fix.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to DAGCombiner: Don't unnecessarily swap operands in ReassociateOps.
tstellarAMD updated this object.
tstellarAMD edited the test plan for this revision. (Show Details)
tstellarAMD added a reviewer: resistor.
tstellarAMD added a subscriber: Unknown Object (MLST).

Adding Tim because this breaks an AArch64 test: logical_shifted_reg.ll. It appears to me the code produced is worse, but I'm not sure. Here is the output from the test, before is on the left, after is on the right:

http://people.freedesktop.org/~tstellar/aarch_logical_shifted_reg.diff

arsenm added a subscriber: arsenm.Jun 5 2015, 6:30 PM
arsenm commandeered this revision.Feb 22 2016, 7:10 PM
arsenm updated this revision to Diff 48767.
arsenm added a reviewer: tstellarAMD.

Update for trunk. The AArch64 test no longer breaks, and only a couple of x86 tests have irrelevant changes

t.p.northover accepted this revision.Feb 23 2016, 9:29 AM
t.p.northover edited edge metadata.

Looks reasonable to me (and would even if it made AArch64 slightly worse, that would have to be coincidence). I've no idea why they were swapped in the first place.

Tim.

This revision is now accepted and ready to land.Feb 23 2016, 9:29 AM
arsenm closed this revision.Feb 27 2016, 12:02 PM

r262148