This is an archive of the discontinued LLVM Phabricator instance.

Remove AND mask of bottom 6 bits when result is used for SystemZ shift/rotate
ClosedPublic

Authored by colpell on Jun 29 2016, 11:54 AM.

Details

Summary

On SystemZ, shift and rotate instructions only use the bottom 6 bits of the shift/rotate amount. Therefore, if the amount is ANDed with an immediate mask that has all of the bottom 6 bits set, we can remove the AND operation entirely.

Diff Detail

Repository
rL LLVM

Event Timeline

colpell updated this revision to Diff 62260.Jun 29 2016, 11:54 AM
colpell retitled this revision from to Remove AND mask of bottom 6 bits when result is used for SystemZ shift/rotate.
colpell updated this object.
colpell added a reviewer: uweigand.
colpell added a subscriber: llvm-commits.
uweigand edited edge metadata.Jun 30 2016, 6:32 AM

I'm not sure that adding so many extra patterns in the .td file is the best way to implement this. Maybe it would be better to implement the transformation of the mask in PerformDAGCombine? This is done earlier, and might in theory expose more combination opportunities ...

colpell updated this revision to Diff 62425.Jun 30 2016, 2:35 PM
colpell edited edge metadata.

Good point, thanks Ulrich. As suggested, I've moved the implementation to a more concise one in PerformDAGCombine.

Looks good in general. However, it seems you missed the recent refactoring to the PerformDAGCombine routine (rev. 274191), so your patch won't apply. Please update to current mainline (your code should now go into a subroutine like combineShift).

As further enhancement I'm wondering if it might be useful to tweak the ANDed constant even if the AND cannot be optimized away completely. Not sure if that really makes a difference to real-world code though.

lib/Target/SystemZ/SystemZISelLowering.cpp
5064 ↗(On Diff #62425)

In principle it ought to be possible to do that optimization even if there are multiple uses of the AND result (the AND would still stay there, just not used for the shift).

colpell updated this revision to Diff 62755.Jul 5 2016, 8:43 AM

Update to latest trunk and remove hasOneUse check.

Update to latest trunk and remove hasOneUse check.

Ah, now it seems the CombineTo use is incorrect (this will replace *all* uses of the AND with its input value, which is incorrect if there is indeed another use). So I think we have to generate a new shift/rotate output node instead of using CombineTo here.

colpell updated this revision to Diff 62868.Jul 6 2016, 7:59 AM

If the AND value has more than one use, generate a new shift/rotate node with the AND's first operand rather than removing the AND completely. Also add tests for this situation.

uweigand accepted this revision.Jul 6 2016, 8:02 AM
uweigand edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jul 6 2016, 8:02 AM
This revision was automatically updated to reflect the committed changes.