This is an archive of the discontinued LLVM Phabricator instance.

DAGCombine: (shl (or x, c1), c2) -> (or (shl x, c2), c1 << c2)
ClosedPublic

Authored by RKSimon on Apr 20 2016, 7:00 AM.

Details

Summary

We already have a combine for this pattern when the input to shl
is add, so we just need to enable the transformation with the input
is or.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to DAGCombine: (shl (or x, c1), c2) -> (or (shl x, c2), c1 << c2).
tstellarAMD updated this object.
tstellarAMD added a reviewer: resistor.
tstellarAMD added a subscriber: llvm-commits.
arsenm added a subscriber: arsenm.Apr 20 2016, 8:58 AM

Needs a test for the multiple use case. Tests separate from the ds_permute would also be good

The pattern was matching correctly but always emitting add in the output dag,
even when int he input was or. This was fixed and I added a test case for this
behavior.

Add some more tests.

RKSimon added a subscriber: RKSimon.May 9 2016, 6:04 AM

Any movement on this?

RKSimon edited edge metadata.Apr 3 2017, 6:36 AM
RKSimon added a subscriber: tstellar.

Tom - do you mind if I commandeer this?

RKSimon commandeered this revision.Jun 10 2017, 1:18 PM
RKSimon edited reviewers, added: tstellarAMD; removed: RKSimon.
RKSimon added reviewers: tstellar, arsenm, efriedma, spatel.
RKSimon updated this revision to Diff 102124.Jun 10 2017, 1:24 PM

Rebaing - this is part of constant canonicalizations to try and combine MUL/SHL ops separated by ADD/OR ops.

If an AMDGPU expert can check the test changes, that'd be great.

tstellar added inline comments.Jun 12 2017, 6:12 AM
test/CodeGen/AMDGPU/fneg-fabs.f16.ll
91–95

Does this test generate extra shl instructions now, or did it generate those before the patch and there just weren't any check lines for it?

RKSimon updated this revision to Diff 113858.Sep 5 2017, 7:48 AM

Refreshed test checks after rL312537 to make the diff clearer

Sorry for the late reply - this fell to the bottom of my todo pile for a while

test/CodeGen/AMDGPU/fneg-fabs.f16.ll
91–95

To confirm, the shifts were always there but the test didn't check for them

efriedma edited edge metadata.Sep 13 2017, 11:50 AM

For reference, instcombine does the same transform in InstCombiner::tryFactorization.

I can't really check the AMDGPU tests; otherwise LGTM.

tstellar accepted this revision.Sep 13 2017, 7:35 PM

LGTM.

This revision is now accepted and ready to land.Sep 13 2017, 7:35 PM
This revision was automatically updated to reflect the committed changes.