[PATCH] D66718: [DAGCombiner] (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC) -> (vector_shuffle X, Y)

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16421
   SDValue InsertVal = N->getOperand(1);
+  SDValue Vec = N->getOperand(0);
+
----------------
lebedev.ri wrote:
> OldShuffle
There's no need to rename - tbh we don't know its a shuffle at this stage!


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16441
+    } else if (InsertVal.getOperand(0) == SV1) {
+      Offset = Mask.size();
+    }
----------------
lebedev.ri wrote:
> Actually, this is not correct.
> While both shuffle inputs must have same types,
> the shuffle mask can have different dimensions.
> So this should be closer to `Offset = X.getType().getVectorNumElements()`.
> 
> https://llvm.org/docs/LangRef.html#shufflevector-instruction
That is only true for IR - in the DAG, the result must have the same type as the shuffle operands - so the number of mask elts must match the number of result/operand elts - see SelectionDAG::getVectorShuffle


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16447
+
+      auto *IndexC = cast<ConstantSDNode>(InsertVal.getOperand(1));
+      NewMask[InsIndex] = Offset + IndexC->getZExtValue();
----------------
lebedev.ri wrote:
> ExtrIndex
Please can you assert that IndexX's value (and InsIndex come to that) are both in bounds (0 <= idx < Mask.size())?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66718