[PATCH] D17041: [X86] Don't assume that a shuffle operand is #0: it isn't for VPERMV.

[Ahmed Bougacha via llvm-commits]

ab added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:23658
@@ -23621,1 +23657,3 @@
+  }
+
   // At the moment we can only combine target shuffle unary cases.
----------------
RKSimon wrote:
> Was there a need to add this? We already have all undef and all undef/zero handling below and in combineX86ShuffleChain
Yes, but before we can figure that out, we only checked undef (see the TODO about zero/undef below), and we crash trying to and getOpcode() on SDValue().

But to be clear: the whole patch is very mechanical, and this is a hack. I'm mainly interested in whether I should proceed with this of swap the operands.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:24335
@@ -24289,3 +24334,3 @@
   SDValue N0 = N->getOperand(0);
   SDValue N1 = N->getOperand(1);
   EVT VT = N->getValueType(0);
----------------
RKSimon wrote:
> Don't these need dealing with as well? Also at the moment the getOperand(1) calls means that unary target shuffles can't use this combine as they would assert....
This actually doesn't get called for all shuffles opcodes, though it probably should.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:24352
@@ -24306,3 +24351,3 @@
       N->getOpcode() == ISD::VECTOR_SHUFFLE)
     return PerformShuffleCombine256(N, DAG, DCI, Subtarget);
 
----------------
And this looks like it should be fall through to the other cases if PerformShuffleCombine256 didn't succeed.


http://reviews.llvm.org/D17041