[PATCH] D83606: [DAGCombiner][WebAssembly] Combine shuffles of more splat vals

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19841
 // Combine shuffles of splat-shuffles of the form:
 // shuffle (shuffle V, undef, splat-mask), undef, M
 // If splat-mask contains undef elements, we need to be careful about
----------------
We should fix this comment too now that we allow non-shuffles in side


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19852
+  APInt DemandedElts =
+      APInt::getAllOnesValue(Shuf->getValueType(0).getVectorNumElements());
+  if (!DAG.isSplatValue(Shuf->getOperand(0), DemandedElts, UndefElts))
----------------
The previous version seems to allow undef elements in splats, but this forces all elements to be demanded. Is this OK?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19902
 static SDValue formSplatFromShuffles(ShuffleVectorSDNode *OuterShuf,
                                      SelectionDAG &DAG) {
   if (!OuterShuf->getOperand(1).isUndef())
----------------
It looks like this method deals with a simpler form in which both inner and outer masks are splats, but it seems this has the same limitation that only shuffles are considered. Can this be improved too? (Not necessarily in this patch though)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1749
+    if (UndefElts.any())
+      return DAG.getSplatBuildVector(N->getValueType(0), SDLoc(N), SplatVal);
+
----------------
The same question as above. The previous version of `combineShuffleOfSplatVal` seems to allow undef lanes from splats. The previous version [[ https://github.com/llvm/llvm-project/blob/8b85f68ee2ddd983c027adbda9567f06d25b3c51/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L19840 | calls ]] `ShuffleVectorSDNode::isSplat`, which calls `ShuffleVectorSDNode::isSplatMask`, which seems to allow undefs. Also the [[ https://github.com/llvm/llvm-project/blob/8b85f68ee2ddd983c027adbda9567f06d25b3c51/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L19847-L19862 | comments ]] there seem to take undef elements in the splat mask in consideration. Do we need to, or, it safe to change it to disallow it? If we allow undef splats in `combineShuffleOfSplatVal`, we don't need this separate target combine function, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83606