This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold (x >> y) << y -> x & (-1 << y)
ClosedPublic

Authored by lebedev.ri on Jun 9 2018, 3:26 AM.

Details

Summary

We already do it for matching splat constants, but not just values.

Further improvements for non-matching splat constants, as noted in
https://reviews.llvm.org/D46760#1123713 will be needed,
but i'd prefer to do that as a follow-up.

https://bugs.llvm.org/show_bug.cgi?id=37603
https://rise4fun.com/Alive/cplX
https://rise4fun.com/Alive/0HF

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 9 2018, 3:26 AM
lebedev.ri updated this revision to Diff 150627.Jun 9 2018, 7:13 AM

Rebase ontop of more tests, simplify matcher, support ashr too.

lebedev.ri edited the summary of this revision. (Show Details)Jun 9 2018, 7:14 AM
spatel accepted this revision.Jun 10 2018, 9:32 AM

LGTM.

lib/Transforms/InstCombine/InstCombineShifts.cpp
608

Remove the duplicate/shadow variable.

This revision is now accepted and ready to land.Jun 10 2018, 9:32 AM

LGTM.

Thank you for the review.

Even in light of https://reviews.llvm.org/D46760#1127287, i believe we still can do *this* transform, ...

lib/Transforms/InstCombine/InstCombineShifts.cpp
608

Oh, hm, i wonder why there was no -Wshadow warning.

620–624

... because we already do it here for when the y is constant, regardless of exactness of shr.
Also, maybe this block should be dropped, now that the more general fold will be done, one not limited to constants?

Even in light of https://reviews.llvm.org/D46760#1127287, i believe we still can do *this* transform, ...

Agreed. I don't think a variable shift amount is a problem, but if it is, then maybe we should enhance SCEV rather than cripple instcombine? (And I think the same could be said for the constant shift amount).

lib/Transforms/InstCombine/InstCombineShifts.cpp
620–624

Right - I saw that block. I think it's a just a matter of taste because we don't need the one-use check if the shift amount is constant (so you'd have to add an 'if' for that possibility in the later block).

Even in light of https://reviews.llvm.org/D46760#1127287, i believe we still can do *this* transform, ...

Agreed. I don't think a variable shift amount is a problem, but if it is, then maybe we should enhance SCEV rather than cripple instcombine?

Yep, absolutely, that is why i CC'd Max. That crippling is just wrong.
To point the obvious, it just kinda ignores all the cases that already were shifts.

(And I think the same could be said for the constant shift amount).

lib/Transforms/InstCombine/InstCombineShifts.cpp
620–624

Oh, right, good point. Will keep it then.

This revision was automatically updated to reflect the committed changes.