[PATCH] D31961: DAGCombine: Combine shuffles of splat-shuffles

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14635-14642
+  for (unsigned i = 0; i < NumElts; ++i)
+    if (UserMask[i] < 0)
+      NewMask.push_back(Splat->getMaskElt(i));
+    else {
+      int NewIdx = Splat->getMaskElt(UserMask[i]);
+      IsSameMask &= NewIdx == Splat->getMaskElt(i);
+      NewMask.push_back(NewIdx);
----------------
I'm scared that I'm not seeing all of the undef possibilities here (since I definitely missed them the first time!).

How about writing this to make the options more visible (not sure if it's more or less efficient), but I'd find this easier to read (warning: untested code):
  // If the splat mask has no undefs, then return it as-is. 
  // It doesn't matter if the user mask has undefs. We choose the splat mask element because it's efficient to return an existing splat node.
  ArrayRef<int> SplatMask = Splat->getMask();
  if (find(SplatMask, -1) == SplatMask.end())
    return SDValue(Splat, 0);
  
  // The splat mask has undefs. If the user shuffle does not choose undef elements where the splat does not have undef elements, return the existing splat node.
  if (all_of(UserMask, [](int Idx) { return Idx == -1 || Splat->getMaskElt(Idx) == Splat->getSplatIndex(); }))
    return SDValue(Splat, 0); 
    
  // The splat mask has undefs, and the user shuffle chooses undef elements where the splat has defined elements. Create a more liberal splat mask (one with more undefs).
   for (unsigned i = 0; i < NumElts; ++i)
    // No need for "IsSameMask" here because we already handled any case where we wanted to return the existing node.


Repository:
  rL LLVM

https://reviews.llvm.org/D31961