[PATCH] D68195: [DAGCombiner] Peek through vector concats when trying to combine shuffles.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Okay, this seems pretty unusual, but correct.
Would be good for @RKSimon to take a look, but LGTM.

I don't have comments as to the algorithm, but i find
the comments/variable names to be somewhat confusing.
Some 'recommendations':



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16472-16473
 
   // (insert_vector_elt (vector_shuffle X, Y), (extract_vector_elt X, N), InsIndex)
-  //   --> (vector_shuffle X, Y)
+  //   --> (vector_shuffle X, Y) and variations.
   if (Vec.getOpcode() == ISD::VECTOR_SHUFFLE && Vec.hasOneUse() &&
----------------
... where shuffle operands may be CONCAT_VECTORS.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16478
     ShuffleVectorSDNode *SVN = cast<ShuffleVectorSDNode>(Vec.getNode());
     ArrayRef<int> Mask = SVN->getMask();
 
----------------
I forget, by now `Mask.size() == Vec.getValueType().getVectorNumElements()`?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16487
+    SDValue InsertVal0 = InsertVal.getOperand(0);
     int XOffset = -1;
+
----------------
This is no longer `X` offset, this is what was most confusing to me.
I think this needs to be renamed to something else.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16513-16514
+          ArgWorkList.emplace_back(ArgOffset, Op);
+        }
+      }
     }
----------------
I think this needs a defensive/explanatory assert that by the end
of the loop `ArgOffset` is back to the original value of `ArgOffset`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68195