[PATCH] D87340: [EarlyCSE] Handle masked loads and stores

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Overall this seems like a natural extension of the existing EarlyCSE handling of load/store.  I don't see any major issues.



================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:710
+            Info.WriteMem = false;
+            Info.IsVolatile = false;
+            break;
----------------
Info.MatchingID is getting implicitly set to zero?  Could that conflict with a target-specific ID?  I guess you explicitly check the identity of the intrinsic later, but it seems a little confusing that you aren't doing anything explicit with getMatchingId().


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:876
+        return true;
+      assert(!isa<UndefValue>(Mask0) && !isa<UndefValue>(Mask1));
+      auto *Vec0 = dyn_cast<ConstantVector>(Mask0);
----------------
Is there any reason to expect `!isa<UndefValue>(Mask0)` to hold here?  I mean, it would be weird to see a load with an undef mask, but I don't see any code that would enforce it, and the assertion itself doesn't explain.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:879
+      auto *Vec1 = dyn_cast<ConstantVector>(Mask1);
+      if (!Vec0 || !Vec1 || Vec0->getType() != Vec1->getType())
+        return false;
----------------
The `Vec0->getType() != Vec1->getType()` shouldn't be necessary: if the values are the same type, that implies the masks are the same type.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:887
+      }
+      return true;
+    };
----------------
There should be some test coverage for the unequal-masks case.

If the unequal-masks cases are important, you might run into cases where a masked operation with an all-true mask got folded to a plain load/store.  Not sure how likely that is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87340