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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryLocation.cpp:154
+  // that we can so summarize.
+  if (!CB->onlyAccessesArgMemory() || !CB->willReturn() || !CB->doesNotThrow())
+    return None;
----------------
reames wrote:
> 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.
This code was only recently moved from DSE into here, and it makes sense that DSE did not check this while it was still working on a hardcoded list of functions. We should already be specifying/inferring the necessary attributes (though possibly the tests don't have the full set of inferred attributes).


================
Comment at: llvm/lib/Analysis/MemoryLocation.cpp:186
+
+  return MemoryLocation::getBeforeOrAfter(UsedV, CB->getAAMetadata());
 }
----------------
Thinking about libcalls, I just realized that this should probably not returning a getBeforeOrAfter(UsedV), but rather remembering which argument the write is on and then calling MemoryLocation::getForArgument() on that. That would likely subsume the libcall cases by returning a more accurate location if getForArgument() implements something more specific.

This would lose the case where the same argument is passed to the function twice, but I don't think we particularly care about that (as it would have to be literally the same argument as implemented).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115904