[PATCH] D115904: [DSE] Remove calls with known writes to dead memory

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D115904#3199421 <https://reviews.llvm.org/D115904#3199421>, @xbolva00 wrote:

> Please check failing ASAN tests

These are almost certainly spurious.  The pre-commit CI picks a random base commit when doing builds.  This change has nothing to do with compiler-rt, so this breakage is probably in the base commit chosen.



================
Comment at: llvm/include/llvm/Analysis/MemoryLocation.h:260
+  /// sunk, reordered, etc... solely based on the memory semantics implied by
+  /// the described write, and the def-use chain of the result value (if any).
   static Optional<MemoryLocation> getForDest(const CallBase *CI,
----------------
nikic wrote:
> This might want to highlight that the function can still read other memory.
I thought the wording about side effects covered this, but will do so if desired. 


================
Comment at: llvm/lib/Analysis/MemoryLocation.cpp:154
+  // that we can so summarize.
+  if (!CB->onlyAccessesArgMemory() || !CB->willReturn() || !CB->doesNotThrow())
+    return None;
----------------
nikic wrote:
> I'm not sure the willreturn and nounwind checks really belong in here. We should try to separate the modelling of different effects, and MemoryLocation should only be modelling memory effects. The willreturn/nounwind checks can be done in the caller.
> 
> After all, the statement that only this one location is written remains true regardless of whether the call unwind or diverge, the latter only affect whether it can be removed.
I'm sympathetic to this concern, but this appears to already be the implicit contract of this method.  In particular, DSE does not otherwise check these properties for any of the intrinsics and libfuncs.

I propose that we go with this, and if desired, try to annotate all the intrinsics/libfuncs separately so that these checks can be lifted out to the two callers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115904