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

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz marked 4 inline comments as done.
kparzysz added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:710
+            Info.WriteMem = false;
+            Info.IsVolatile = false;
+            break;
----------------
efriedma wrote:
> 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().
Setting it to `masked_load` (with an explanation in a comment).


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:876
+        return true;
+      assert(!isa<UndefValue>(Mask0) && !isa<UndefValue>(Mask1));
+      auto *Vec0 = dyn_cast<ConstantVector>(Mask0);
----------------
efriedma wrote:
> 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.
You're right, I changed it to an explicit check.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:879
+      auto *Vec1 = dyn_cast<ConstantVector>(Mask1);
+      if (!Vec0 || !Vec1 || Vec0->getType() != Vec1->getType())
+        return false;
----------------
efriedma wrote:
> The `Vec0->getType() != Vec1->getType()` shouldn't be necessary: if the values are the same type, that implies the masks are the same type.
There is a check if the pointer operands are equal, so you're right, the types here are implied to be the same.  I added it as a safeguard for when that check (which is at another place) is relaxed to checking that the pointers point to the same location, but are not necessarily equal as Value's.  I changed the type check here to an assertion, so this will still fail when this condition is accidentally violated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87340