[PATCH] D96345: [DAG] Fold shuffle(binop(shuffle(x, y), shuffle(z, w))) -> binop(shuffle(a,b),shuffle(c,d))

[Pengfei Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20981
+  // other shuffles.
+  // shuffle(bop(shuffle(x,y),shuffle(z,w)),bop(shuffle(a,b),shuffle(c,d)))
+  if (Level < AfterLegalizeDAG && TLI.isTypeLegal(VT)) {
----------------
Will the merge break some thing like expection handling? E.g. Under strict FP model, we should make sure no unexpected expections raised by the masked elements.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20983
+  if (Level < AfterLegalizeDAG && TLI.isTypeLegal(VT)) {
+    unsigned SrcOpcode = N->getOperand(0).getOpcode();
+    if (SrcOpcode == N->getOperand(1).getOpcode() && TLI.isBinOp(SrcOpcode) &&
----------------
Can we use a variable to save `N->getOperand(0/1)`? They are used many times later.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:20984
+    unsigned SrcOpcode = N->getOperand(0).getOpcode();
+    if (SrcOpcode == N->getOperand(1).getOpcode() && TLI.isBinOp(SrcOpcode) &&
+        N->isOnlyUserOf(N->getOperand(0).getNode()) &&
----------------
Should it be `isCommutativeBinOp` for `Op10` and `Op11`?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21002-21005
+        if (Op00.getOpcode() == ISD::VECTOR_SHUFFLE &&
+            N->getOperand(0)->isOnlyUserOf(Op00.getNode()) &&
+            MergeInnerShuffle(false, SVN, cast<ShuffleVectorSDNode>(Op00), Op10,
+                              TLI, LeftSV0, LeftSV1, LeftMask)) {
----------------
Can we use a lambda expression to save the repeated code?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21007
+          MergedLeft = true;
+        } else if (Op10.getOpcode() == ISD::VECTOR_SHUFFLE &&
+                   N->getOperand(0)->isOnlyUserOf(Op10.getNode()) &&
----------------
How about both `Op00` and `Op10` are VECTOR_SHUFFLE? Do we miss the chance for `Op10`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96345