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

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14880
+  ArrayRef<int> SplatMask = Splat->getMask();
+  assert(UserMask.size() == SplatMask.size() && "Mask length mismatxh");
+
----------------
spatel wrote:
> typo: mismatxh
Thanks


================
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;
+      }())
----------------
spatel wrote:
> 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);
Ok


================
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;
----------------
spatel wrote:
> 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.
I agree this will improve the readability of the code and point-out this tricky subtlety.


https://reviews.llvm.org/D31961