Page MenuHomePhabricator

[InstSimplify] delete shift-of-zero guard ops around funnel shifts
ClosedPublic

Authored by spatel on Nov 14 2018, 3:41 PM.

Details

Summary

This is a problem seen in common rotate idioms as noted in:
https://bugs.llvm.org/show_bug.cgi?id=34924

Note that we are not canonicalizing standard IR (shifts and logic) to the intrinsics yet. (Although I've written this before...) I think this is the last step before we enable that transform. Ie, we could regress code by doing that transform without this simplification in place.

In PR34924, I questioned whether this is a valid transform for target-independent IR, but I convinced myself this is ok. If we're speculating a funnel shift by turning cmp+br into select, then SimplifyCFG has already determined that the transform is justified. It's possible that SimplifyCFG is not taking into account profile or other metadata, but if that's true, then it's a bug independent of funnel shifts.

Also, we do have CGP code to restore a guard like this around an intrinsic if it can't be lowered cheaply. But that isn't necessary for funnel shift because the default expansion in SelectionDAGBuilder includes this same cmp+select.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Nov 14 2018, 3:41 PM
fabiang accepted this revision.Nov 14 2018, 4:34 PM

Looks correct to me.

I don't see any issues with doing this at the target-independent level. It could be a problem if fshl/fshr had tricky boundary cases, but their shift amount is taken to be modulo bitwidth and the ShAmt=0 case is explicitly legal and well-defined.

This revision is now accepted and ready to land.Nov 14 2018, 4:34 PM
This revision was automatically updated to reflect the committed changes.