Page MenuHomePhabricator

[SelectionDAGBuilder] Enable funnel shift building to custom rotates

Authored by RKSimon on Dec 16 2018, 12:34 PM.



This patch enables funnel shift -> rotate building for all ROTL/ROTR custom/legal operations.

AFAICT X86 was the last target that was missing modulo support (PR38243), but I've tried to CC stakeholders for every target that has ROTL/ROTR custom handling for their final OK.

Diff Detail


Event Timeline

RKSimon created this revision.Dec 16 2018, 12:34 PM
spatel added inline comments.Dec 17 2018, 2:32 PM
5765–5766 ↗(On Diff #178407)

Can remove the TODO comment with this change.

94–96 ↗(On Diff #178407)

Is it worth repeating some of the code comments here to describe how this works?
Shouldn't we have coverage for v8i16 and 256/512-bit custom types?

RKSimon marked an inline comment as done.Dec 17 2018, 2:48 PM
RKSimon added inline comments.
94–96 ↗(On Diff #178407)

What do you think is the best way forward for vector test coverage? We already have the vector-rotate-*.ll tests (which are raw shl+lshr+or patterns) - should those be replaced with fshl/fshr "rotate" intrinsics?

RKSimon updated this revision to Diff 178630.Dec 18 2018, 2:50 AM

Rebased with the new vector test coverage

spatel added inline comments.Dec 18 2018, 7:03 AM
94–96 ↗(On Diff #178407)

What we have now with the updated patch is good: we want coverage for the intrinsics regardless of what we do with the "legacy" patterns. I'd think other targets should do the same, but I'll leave that decision to the respective owners.

We can mark the old vector rotate tests as deprecated once we start canonicalizing IR to the funnel-shift intrinsics. It is still possible to create scalar rotates via bswap and possibly scalar or vector rotate through logic/shift combines, so we'll probably never get rid of the old tests completely.

spatel added inline comments.Dec 18 2018, 11:35 AM
550–554 ↗(On Diff #178709)

This looks like a regression for AVX512?

RKSimon updated this revision to Diff 179068.Dec 20 2018, 6:49 AM

rebased now that avx512 avoids lowering v16i8 rotations (expansion to wider shifts is better).

spatel accepted this revision.Dec 20 2018, 6:56 AM


This revision is now accepted and ready to land.Dec 20 2018, 6:56 AM
This revision was automatically updated to reflect the committed changes.