This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Shift amount reassociation (PR42391)
ClosedPublic

Authored by lebedev.ri on Jun 26 2019, 2:51 AM.

Details

Summary

Given pattern:
(x shiftopcode Q) shiftopcode K
we should rewrite it as
x shiftopcode (Q+K) iff (Q+K) u< bitwidth(x)
This is valid for any shift, but they must be identical.

Should fix PR42391.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic added inline comments.Jun 26 2019, 10:01 AM
lib/Transforms/InstCombine/InstCombineShifts.cpp
37

Nice typo ;)

lebedev.ri added inline comments.Jun 26 2019, 10:12 AM
lib/Transforms/InstCombine/InstCombineShifts.cpp
37

Sigh, i thought i only had this in next WIP patch, apparently i copied it from here :/

lebedev.ri marked 2 inline comments as done.

Rebased

nikic added inline comments.Jun 27 2019, 11:52 AM
lib/Transforms/InstCombine/InstCombineShifts.cpp
30

Should probably be const SimplifyQuery &SQ?

51

Would it be possible to preserve nowrap/exact flags that exist on both shifts?

lebedev.ri added inline comments.Jun 27 2019, 12:17 PM
lib/Transforms/InstCombine/InstCombineShifts.cpp
51

So basically if the same flag is set on both original shifts -> set it on new shift.
Don't think we can do anything with non-matching flags on shl.

lebedev.ri marked 3 inline comments as done.
lebedev.ri edited the summary of this revision. (Show Details)

Preserve flag (nuw/nsw/exact) if both shifts had it.
PTAL.

nikic accepted this revision.Jun 29 2019, 4:15 AM

LGTM

lib/Transforms/InstCombine/InstCombineShifts.cpp
37

Personally I'd prefer auto *Sh1 = cast<BinaryOperator>(Sh0->getOperand(0)) over the use of m_CombineAnd() here, which makes the match less straightforward.

41

Can replace cast<Instruction>(Sh0->getOperand(0)) with Sh1 here.

This revision is now accepted and ready to land.Jun 29 2019, 4:15 AM
lebedev.ri marked 2 inline comments as done.EditedJun 29 2019, 4:30 AM

Thanks for the review!

Other than D63829, unless that is magically sufficient to fix all the
performance problems i'm observing, i suspect i will have some more
folds like this...

lib/Transforms/InstCombine/InstCombineShifts.cpp
37

It really differs on case-by-case basis. I think m_CombineAnd() may be better in general,
at least compared to backend lack of it.

41

Whoops, yes, i wanted to do that but forgot. Thank you.

This revision was automatically updated to reflect the committed changes.