[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:14880
+  ArrayRef<int> SplatMask = Splat->getMask();
+  assert(UserMask.size() == SplatMask.size() && "Mask length mismatxh");
+
----------------
typo: mismatxh


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14886-14892
+  if ([UserMask, SplatMask]() {
+        for (unsigned i = 0, e = UserMask.size(); i != e; ++i)
+          if (UserMask[i] != -1 && SplatMask[i] == -1 &&
+              SplatMask[UserMask[i]] != -1)
+            return false;
+        return true;
+      }())
----------------
I'm a coding caveman, so I had not seen this syntax before. I know it takes a line or 2 more, but I think it's easier to read when you give the lambda a name, so something like:

  auto canSimplifyToExistingSplat = [UserMask, SplatMask]() { ... }
  if (canSimplifyToExistingSplat())
    return SDValue(Splat, 0);


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14887-14890
+        for (unsigned i = 0, e = UserMask.size(); i != e; ++i)
+          if (UserMask[i] != -1 && SplatMask[i] == -1 &&
+              SplatMask[UserMask[i]] != -1)
+            return false;
----------------
Given that I got this wrong twice, I'd like to see some mask examples for various undef cases in the code comments. I think the tests cover these, so it should just be a matter of listing those cases here similar to the explanatory note in this review:
  UserMask = [0,2,u,u], SplatMask=[2,u,2,u] --> ? 

Bonus points for adding similar comments in the test file to describe the undef handling there too.


https://reviews.llvm.org/D31961