[PATCH] D92045: [DSE] Consider out-of-bound writes in isOverwrite.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:500
+      // if the writes are fully inside the underlying object, because the
+      // out-of-bounds access is guaranteed to execute. Note that the case when
+      // the block is exited early due to unwinding is already handled
----------------
nikic wrote:
> Why is it guaranteed to execute? Can't you have `call @infinite_loop()` in between?
Hm, there might be potential to improve the wording. I think if any call in between does not return, we only eliminate if the call does not access the location in question, otherwise it would be clobber blocking DSE. If the call does not return, it should be legal to eliminate the earlier store, because the call does not read the location. Otherwise the second store will be executed. 

Am I missing something? In that case the code might require an audit now that we have `!mustprogress`. I think the code was mostly written with `!mustprogress` assumed.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/out-of-bounds-stores.ll:73
   ret i32 0
 }
----------------
nikic wrote:
> I'm having a hard time understanding why anything gets eliminated in this example, regardless of whether this is considered an overwrite or not. There is a codepath (for.body -> for.inc -> for.end) that goes from the first store to the load without passing through the store in for.body.1, so how can it even be relevant for DSE purposes?
The problem here is that we start looking upwards with the location of the potential killing definition. So when going upwards we consider `store i32 10, i32* %arrayidx, align 4` to be completely overwritten by `store i32 20, i32* %arrayidx.1, align 4`, which is out-of-bounds. We then check for read clobbers starting at `store i32 10, i32* %arrayidx, align 4`, we use the location of the killing def, which does not alias the read.

It might be beneficial to use the location of the store to eliminate for the read check for the full overwrite case at least. But I think that would be better as follow up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92045