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

[Matteo Favaro via Phabricator via llvm-commits]

fvrmatteo added a comment.

> It seems these two fixes may be independent, but you can check my GVN approach in the diff: https://reviews.llvm.org/D93529?id=313082.

I'm reading the GVN source code to learn about it and understand your approach. I'll try to support the dead store example you shared. I'm probably missing something, but it seems the GVN approach is based on information deriving from the Memory Dependence Analysis (and related API), while here I'd like to find a solution suitable for both MD and MSSA in the isOverwrite function, hence I'll try to generalise the check to support the vector types.

> Just some food for thought in case anyone is interested in tackling this issue :)

Assuming I'll be able to come up with a proper solution to this issue, that surely would be another occasion to learn something more about LLVM.

I'll update the patch as soon as I can figure out a reasonable way to support the different types and will update the test files accordingly.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:467
+  // Query the alias information
+  AliasResult AAR = AA.alias(Later, Earlier);
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:471
   // the later store was larger than the earlier store.
-  if (P1 == P2 || AA.isMustAlias(P1, P2)) {
+  if (P1 == P2 || (AAR == AliasResult::MustAlias)) {
     // Make sure that the Later size is >= the Earlier size.
----------------
nikic wrote:
> As you are performing the alias query unconditionally now, the separate `P1 == P2` shortcut here isn't useful anymore, you can drop it. (I also checked whether your change may have a compile-time impact due to the unconditional query, but it does not seem so.)
Indeed, thanks for pointing it out. Glad that for now the compile-time impact is negligible.


================
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:
> 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.


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

https://reviews.llvm.org/D97676