[PATCH] D97676: [DSE] Extending isOverwrite to support offsetted fully overlapping stores

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:467
+  // Query the alias information
+  AliasResult AAR = AA.alias(Later, Earlier);
+
----------------
fvrmatteo wrote:
> nikic wrote:
> > Why not continue using P1 and P2 here? Of course AA will also strip pointer casts, but if we already did it, we may as well pass the stripped pointers.
> I agree I should avoid doing the same action multiple times, although I used `Later` and `Earlier` because `BatchAAResults` is proving an `alias()` function expecting `MemoryLocation` arguments.
Oh right, that makes sense. You might want to move the P1/P2 declarations lower in the code, where they are needed.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1314
   const DataLayout &DL = BB.getModule()->getDataLayout();
+  BatchAAResults BatchAA(*AA, /*CacheOffsets =*/true);
   bool MadeChange = false;
----------------
nikic wrote:
> fvrmatteo wrote:
> > nikic wrote:
> > > Is BatchAA safe to use in the non-MSSA DSE?
> > I'm not sure it's safe because on the MD-based DSE I see that the IR is modified and the BatchAAResults should be used when there are no IR changes in between queries. At the same time plain AliasAnalysis is not providing the offset information, so I guess I should be dropping BatchAA here for now, even though the isOverwrite will try to call `getClobberOffset` on the AA reference.
> Some types of IR modifications (removing instructions if no new ones are added) can be okay, and the MSSA-based implementation uses that. I'm just not familiar with the old one. Maybe we can avoid the question altogether though, by just deleting this code? D97877
This code is gone now, can you please rebase?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:480
+    int64_t Off = AA.getClobberOffset(Later, Earlier).getValueOr(0);
+    if ((Off > 0) && (((uint64_t)Off + EarlierSize) <= LaterSize))
+      return OW_Complete;
----------------
nit: Unnecessary parentheses.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/offsetted-overlapping-stores.ll:47
+  %i9 = getelementptr inbounds float, float* %arg, i64 %i8
+  store float undef, float* %i9, align 4
+  %i2 = zext i32 %i to i64
----------------
Please store something other than undef here and below (0.0 is fine).


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

https://reviews.llvm.org/D97676