[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 added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16432-16433
+
+  // (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), IdxC)
+  //   --> (vector_shuffle X, Y)
+  if (Vec.getOpcode() == ISD::VECTOR_SHUFFLE && Vec.hasOneUse() &&
----------------
Do you check that both vector_shuffle and extract_vector_elt have the same first argument?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16434-16436
+  if (Vec.getOpcode() == ISD::VECTOR_SHUFFLE && Vec.hasOneUse() &&
+      InsertVal.getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
+      isa<ConstantSDNode>(InsertVal.getOperand(1))) {
----------------
Here we have two instructions, and only require that the first one is single-use.
Observation: we only create a single new instruction.
Do we want to either require both of them be single-use, the first one, either one of them, or none of them?

Another observation - you end up keeping (using) the instruction you just checked for extra uses.
Did you mean to check the other one instead?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16448-16450
+      SmallVector<int, 16> NewMask(Mask.size());
+      for (size_t i = 0; i < Mask.size(); i++)
+        NewMask[i] = Mask[i];
----------------
So `NewMask` is simply inited as a copy of `SVN->getMask()`?
Would something like `SmallVector<int, 16> NewMask(SVN->getMask());` just work?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16452
+
+      auto *IndexC = dyn_cast<ConstantSDNode>(InsertVal.getOperand(1));
+      NewMask[InsIndex] = Offset + IndexC->getZExtValue();
----------------
You either need to check that the cast succeeded, or drop `dyn_` prefix


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16453
+      auto *IndexC = dyn_cast<ConstantSDNode>(InsertVal.getOperand(1));
+      NewMask[InsIndex] = Offset + IndexC->getZExtValue();
+
----------------
This could use a comment


================
Comment at: test/CodeGen/X86/madd.ll:1901-1903
-; AVX512-NEXT:    vpextrd $3, %xmm0, %eax
-; AVX512-NEXT:    vpshufd {{.*#+}} xmm0 = xmm0[1,1,2,3]
-; AVX512-NEXT:    vpinsrd $1, %eax, %xmm0, %xmm0
----------------
If i'm reading this correctly this does showcase the need to handle this in backend - it can be introduced there?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66718