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

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix marked 5 inline comments as done.
deadalnix 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() &&
----------------
lebedev.ri wrote:
> Do you check that both vector_shuffle and extract_vector_elt have the same first argument?
This is checked bellow. It is because we can match either the first or the second argument, as vector_shuffle does in fact shuffle two vectors together.


================
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))) {
----------------
lebedev.ri wrote:
> 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?
Even if InsertVal is reused, this transform reduces data dependencies, but indeed do not save anything on the instruction count from. Removing data dependencies is beneficial on its own.


================
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];
----------------
lebedev.ri wrote:
> So `NewMask` is simply inited as a copy of `SVN->getMask()`?
> Would something like `SmallVector<int, 16> NewMask(SVN->getMask());` just work?
I asked mys elf the same question. This doesn't seem to work.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16452
+
+      auto *IndexC = dyn_cast<ConstantSDNode>(InsertVal.getOperand(1));
+      NewMask[InsIndex] = Offset + IndexC->getZExtValue();
----------------
lebedev.ri wrote:
> You either need to check that the cast succeeded, or drop `dyn_` prefix
The cast must succeed, as we check for it in the if guarding this code, so I'll remove dyn_


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66718