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

[Elena Demikhovsky via llvm-commits]

delena added a comment.

Hi Simon,

I'd write X86 specific combiner for vector rotation because

- VPROT supports 8 bits immediates only
- AVX-512 supports variable rotation

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


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3906
@@ -3907,1 +3905,3 @@
+    uint64_t RShVal = isConstOrConstSplat(RHSShiftAmt)->getZExtValue();
+    if ((LShVal + RShVal) != EltSizeInBits)
       return nullptr;
----------------
Intel XOP set allows only 8 bit immediate even EltSizeInBits=64.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18845
@@ +18844,3 @@
+  // +ve/-ve Amt = rotate left/right.
+  if (Subtarget->hasXOP()) {
+    // Split 256-bit integers.
----------------
I suppose you can put assert(Subtarget->hasXOP()).

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18852
@@ +18851,3 @@
+
+    // Attempt to rotate by immediate.
+    if (auto *BVAmt = dyn_cast<BuildVectorSDNode>(Amt)) {
----------------
One "if" is enough here.
if (auto *RotateConst = Amt->getConstantSplatNode())

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


Repository:
  rL LLVM

http://reviews.llvm.org/D13851