This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] try harder to convert funnel shift to rotate
ClosedPublic

Authored by spatel on Jul 31 2018, 11:22 AM.

Details

Summary

Similar to rL337966 - if the DAGCombiner's rotate matching was working as expected,
I don't think we'd see any test diffs here.

AArch only goes right, and PPC only goes left. x86 has both, so no diffs there. If there are test suggestions for other targets, please let me know.

Diff Detail

Event Timeline

spatel created this revision.Jul 31 2018, 11:22 AM
spatel planned changes to this revision.EditedAug 1 2018, 8:51 AM

There's a subtle bug in the existing code. The conversion we're using to avoid the select for rotates (1st 2 args are the same, X == Y) only works for pow-of-2 bitwidths:
https://rise4fun.com/Alive/US1r
I can't tell yet if that bug was visible in the codegen for the existing non-pow-of-2 tests, and we just overlooked it.

I'll fix that and update this after.

spatel updated this revision to Diff 158572.Aug 1 2018, 10:44 AM

Patch updated:
Rebased after the bug fix and improvements from rL338592.
The code change is smaller and there are less test diffs.

spatel added inline comments.Aug 1 2018, 10:49 AM
test/CodeGen/PowerPC/funnel-shift-rot.ll
159–160

This is weird (independent of this patch) - rldcl 3,3,4,0 is rotld 3,3,4.

We used the extended mnemonic for rotate before, but now we failed to print that?

kparzysz added inline comments.Aug 2 2018, 3:26 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5700

Can this be legal-or-custom (and same at line 5707)?

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5700

see the TODO above:

// TODO: This should also be done if the operation is custom, but we have
// to make sure targets are handling the modulo shift amount as expected.
spatel added inline comments.Aug 5 2018, 9:00 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5700

To add to Simon’s answer, there are known issues with x86 vector lowering discussed in:
https://bugs.llvm.org/show_bug.cgi?id=38243

...so there are many steps left towards standardizing on these intrinsics, but this small one is hopefully ok as-is.

kparzysz accepted this revision.Aug 9 2018, 7:34 AM
This revision is now accepted and ready to land.Aug 9 2018, 7:34 AM
This revision was automatically updated to reflect the committed changes.