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

[Elena Demikhovsky via llvm-commits]

delena added a comment.

Could you, please, add a test with variable rotation-right that requires the "sub" ?
One test with negative immediate.
And one test for 256-bit vector, just to cover all lines that you added.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3906
@@ -3907,1 +3905,3 @@
+    uint64_t RShVal = isConstOrConstSplat(RHSShiftAmt)->getZExtValue();
+    if ((LShVal + RShVal) != EltSizeInBits)
       return nullptr;
----------------
RKSimon wrote:
> 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.
Agree. thank you.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:18856
@@ +18855,3 @@
+        APInt RotateAmt = RotateConst->getAPIntValue();
+        RotateAmt = RotateAmt.zextOrTrunc(8);
+        RotateAmt = Opc == ISD::ROTL ? RotateAmt : -RotateAmt;
----------------
RKSimon wrote:
> 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?
Yes, I suppose you don't need zextOrTrunc() here. Just extract the integer and check the size with assert.


Repository:
  rL LLVM

http://reviews.llvm.org/D13851