[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:16434-16436
+  if (Vec.getOpcode() == ISD::VECTOR_SHUFFLE && Vec.hasOneUse() &&
+      InsertVal.getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
+      isa<ConstantSDNode>(InsertVal.getOperand(1))) {
----------------
deadalnix wrote:
> 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.
So, do we need that one-use restriction here then?


================
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];
----------------
craig.topper wrote:
> deadalnix wrote:
> > 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.
> Yeah SmallVector doesn't take an ArrayRef, but you can do
> 
> SmallVector<int, 16> NewMask(Mask.begin(), Mask.end());
`SmallVector<int, 16> NewMask(SVN->getMask().begin(), SVN->getMask().end());` then.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66718