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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon marked an inline comment as done.
RKSimon 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)) {
----------------
pengfei wrote:
> 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.
We're pushing the shuffle(binop(x,y),binop(z,w)) -> binop(shuffle(x,y),shuffle(z,w))  - so we shouldn't be calling binop on any vector element that it wasn't already. The only danger is if the outer shuffle mask will introduce any undefined elements that the binop can't handle. I'll add a check that ensures we don't merge to a shuffle mask that introduces undefined elements.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:21007
+          MergedLeft = true;
+        } else if (Op10.getOpcode() == ISD::VECTOR_SHUFFLE &&
+                   N->getOperand(0)->isOnlyUserOf(Op10.getNode()) &&
----------------
pengfei wrote:
> How about both `Op00` and `Op10` are VECTOR_SHUFFLE? Do we miss the chance for `Op10`?
We don't change LHS/RHS ordering of any binop elements so we should be OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96345