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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Actually, i have one concern.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16421
   SDValue InsertVal = N->getOperand(1);
+  SDValue Vec = N->getOperand(0);
+
----------------
OldShuffle


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16431-16432
+
+    SDValue SV0 = Vec.getOperand(0);
+    SDValue SV1 = Vec.getOperand(1);
+
----------------
Just x/y like in comments?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16437
+    // elements in the vectors.
+    int Offset = -1;
+    if (InsertVal.getOperand(0) == SV0) {
----------------
Maybe XOffset?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16441
+    } else if (InsertVal.getOperand(0) == SV1) {
+      Offset = Mask.size();
+    }
----------------
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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66718