This is an archive of the discontinued LLVM Phabricator instance.

[NFC][SCEV] Add tests related to bit masking (PR37793)
ClosedPublic

Authored by lebedev.ri on Jun 15 2018, 11:06 AM.

Details

Summary

Related to https://bugs.llvm.org/show_bug.cgi?id=37793, https://reviews.llvm.org/D46760#1127287

We'd like to do this canonicalization https://rise4fun.com/Alive/Gmc
But it is currently restricted by rL155136 / rL155362, which says:

// This is a constant shift of a constant shift. Be careful about hiding
// shl instructions behind bit masks. They are used to represent multiplies
// by a constant, and it is important that simple arithmetic expressions
// are still recognizable by scalar evolution.
//
// The transforms applied to shl are very similar to the transforms applied
// to mul by constant. We can be more aggressive about optimizing right
// shifts.
//
// Combinations of right and left shifts will still be optimized in
// DAGCombine where scalar evolution no longer applies.

I think these tests show that for *constants*, SCEV has no issues with that canonicalization.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 15 2018, 11:06 AM
lebedev.ri added inline comments.Jun 15 2018, 11:22 AM
test/Analysis/ScalarEvolution/lshr-shl-differentconstmask.ll
44–45 ↗(On Diff #151535)

@spatel BTW, how do we want to canonicalize it? https://rise4fun.com/Alive/mxm

72–73 ↗(On Diff #151535)

Similarly, mask first or last? https://rise4fun.com/Alive/0X9

efriedma added inline comments.
test/Analysis/ScalarEvolution/lshr-shl-differentconstmask.ll
44–45 ↗(On Diff #151535)

DAGCombine currently canonicalizes to srl+and. Unless there's a compelling reason to choose something else, that seems fine.

lebedev.ri added inline comments.Jun 15 2018, 12:49 PM
test/Analysis/ScalarEvolution/lshr-shl-differentconstmask.ll
44–45 ↗(On Diff #151535)

Aha, instcombine actually transforms it so that and is last.
https://godbolt.org/g/3cM8D7 (at least for some masks.)
Ok, that answers it.

Actually drop the no-longer-valid comment.

spatel added inline comments.Jun 18 2018, 9:20 AM
lib/Transforms/InstCombine/InstCombineShifts.cpp
625–629 ↗(On Diff #151646)

Better to just add a 'FIXME' note until we actually make the code change here to ignore 'exact'?

lebedev.ri added inline comments.Jun 18 2018, 9:23 AM
lib/Transforms/InstCombine/InstCombineShifts.cpp
625–629 ↗(On Diff #151646)

Yes, indeed. Will add.

lebedev.ri marked 2 inline comments as done.

Add new comment in place of the one being deleted.

lib/Transforms/InstCombine/InstCombineShifts.cpp
625–629 ↗(On Diff #151646)

Hopefully this is better.

This revision is now accepted and ready to land.Jun 20 2018, 12:18 AM

LGTM

Thank you for the review!

lib/Transforms/InstCombine/InstCombineShifts.cpp
625 ↗(On Diff #151747)

This obviously should be // FIXME: we do not yet transform non-exact shr's. The backend (DAGCombine)
Will fix before commit.

This revision was automatically updated to reflect the committed changes.