[PATCH] D55747: [SelectionDAGBuilder] Enable funnel shift building to custom rotates

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: test/CodeGen/X86/funnel-shift-rot.ll:94-96
 ; Vector rotate.
 
 define <4 x i32> @rotl_v4i32(<4 x i32> %x, <4 x i32> %z) nounwind {
----------------
RKSimon wrote:
> spatel wrote:
> > 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?
> 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?
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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55747/new/

https://reviews.llvm.org/D55747