[PATCH] D13851: [X86][XOP] Add support for lowering vector rotations

[Simon Pilgrim via llvm-commits]

RKSimon added a comment.

In http://reviews.llvm.org/D13851#269982, @delena wrote:

> Hi Simon,
>
> I'd write X86 specific combiner for vector rotation because
>
> - VPROT supports 8 bits immediates only
> - AVX-512 supports variable rotation


I'm not sure I agree - AMD XOP has both (128-bit) vector rotation by variable (per element not like SSE2 shifts) and by immediate (same immediate value for all elements) - similar enough to AVX512 but also pretty generic.

The changes to DAGCombiner are limited on purpose, they're just introducing basic vector type support to rotation pattern matching code that at the moment only works for scalars. All of the DAGCombiner changes will work fine for XOP / AVX512 on x86 targets, and could easily be used for other targets (I mentioned ALTIVEC but there may be others too).

> And getConstantSplatNode() does not allows "undef". Do you know this?


Yes - I just didn't want to overload this patch with changes to the DAGCombiner as we don't yet have any targets that lower rotations of vector types (with which to test it), the changes were just enough to get it working for basic examples. With that in place I can then extend the DAGCombiner changes to be handle both all cases of scalar and vector types.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3906
@@ -3907,1 +3905,3 @@
+    uint64_t RShVal = isConstOrConstSplat(RHSShiftAmt)->getZExtValue();
+    if ((LShVal + RShVal) != EltSizeInBits)
       return nullptr;
----------------
delena wrote:
> Intel XOP set allows only 8 bit immediate even EltSizeInBits=64.
This is referring to the size of the scalar element to check for a rotation pattern from 2 shifts - it has nothing to do with instruction sizes and isn't target specific.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18845
@@ +18844,3 @@
+  // +ve/-ve Amt = rotate left/right.
+  if (Subtarget->hasXOP()) {
+    // Split 256-bit integers.
----------------
delena wrote:
> I suppose you can put assert(Subtarget->hasXOP()).
OK - as I said I was trying to setup for you guys to be able to easily add AVX512 support.  

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18856
@@ +18855,3 @@
+        APInt RotateAmt = RotateConst->getAPIntValue();
+        RotateAmt = RotateAmt.zextOrTrunc(8);
+        RotateAmt = Opc == ISD::ROTL ? RotateAmt : -RotateAmt;
----------------
delena wrote:
> I think that the value should be positive here. You already know the rotate direction. And it should fit in 8 bits. 
Would you prefer I add an assert to prove this?


Repository:
  rL LLVM

http://reviews.llvm.org/D13851