This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] canonicalize funnel shift constant shift amount to be modulo bitwidth
ClosedPublic

Authored by spatel on Mar 14 2019, 11:03 AM.

Details

Summary

The shift argument is defined to be modulo the bitwidth, so if that argument is a constant, we can always reduce the constant to its minimal form to allow better CSE and other follow-on transforms.

There was already a test that shows a follow-on simplification, so I assume we just had not made our way around to adding this transform yet.

Diff Detail

Event Timeline

spatel created this revision.Mar 14 2019, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 11:03 AM
lebedev.ri accepted this revision.Mar 14 2019, 11:13 AM

Hmm, this looks good, but i have a nit.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2000

Will m_Constant() also math constant expressions?
Maybe add some test that we don't do anything stupid with them?

This revision is now accepted and ready to land.Mar 14 2019, 11:13 AM
spatel marked an inline comment as done.Mar 14 2019, 11:18 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2000

Yes, that'll match a ConstantExpr...and yes, we often get into trouble with those, so good catch!
I'll add some tests.

You might also want to kill this TODO: https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/InstructionSimplify.cpp#L4920

Technically it's not quite solved because we're not guaranteed canonical IR, but it's probably not worth dealing with this in InstSimplify if we have this in InstCombine.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2012

Can also drop this BitWidth.

spatel marked an inline comment as done.Mar 14 2019, 11:40 AM

You might also want to kill this TODO: https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/InstructionSimplify.cpp#L4920

Technically it's not quite solved because we're not guaranteed canonical IR, but it's probably not worth dealing with this in InstSimplify if we have this in InstCombine.

Agreed - there's almost no chance that other passes will run before instcombine and/or create non-canonical funnel shifts. Thanks for the quick reviews!

This revision was automatically updated to reflect the committed changes.