[PATCH] D109844: [DSE] Track earliest escape, use for loads in isReadClobber.

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1316
+      auto *DefUO = getUnderlyingObject(DefLoc.Ptr);
+      if (DefUO && isa<AllocaInst>(DefUO) && ReadUO && isa<LoadInst>(ReadUO) &&
+          notCapturedBefore(DefUO, UseInst)) {
----------------
fhahn wrote:
> nikic wrote:
> > fhahn wrote:
> > > nikic wrote:
> > > > Use `isIdentifiedFunctionLocal()` and `isEscapeSource()` here? Or else note that this is a compile-time optimization. I think we should be at least using isEscapeSource() though.
> > > Good point, I'll update it to use `isIdentifiedFunctionLocal `. It shouldn't be much more expensive in the grand scheme of things.
> > What about using isEscapeSource() in place of the `isa<LoadInst>` check?
> I can take look at that, but I'll need to make it accessible first. Happy to do that here in this patch or as follow up/
A followup is fine with me.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1292
+    return I != Iter.first->second &&
+           !isPotentiallyReachable(Iter.first->second, I, nullptr, &DT, &LI);
+  }
----------------
It might be worthwhile to add a block reachability cache in the future (possibly that would help with D110094?)


================
Comment at: llvm/test/Transforms/DeadStoreElimination/captures-before-load.ll:127
   %in.lv.1 = load i32* , i32** %in.ptr, align 2
   call void @escape_writeonly(i32* %a)
   %in.lv.2 = load i32 , i32* %in.lv.1, align 2
----------------
To check my understanding, it would be fine to optimize this case right? That is, what we actually care about is not an escape before the load (`%in.lv.2`), but before the instruction producing the loaded value (`%in.lv.1`). That is, our context instruction could be `ReadUO` rather than `UseInst`.

If that's correct, then we might be able to sink this new functionality into AA proper (e.g. via a BatchAA flag for more expensive capture analysis). I previously thought this wasn't possible because alias() does not have a context instruction, but with this we don't actually need one.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/captures-before-load.ll:134
 
 define i32 @test_captured_before_load_same_bb_2(i32** %in.ptr) {
 ; CHECK-LABEL: @test_captured_before_load_same_bb_2(
----------------
Isn't this test case the same as `test_captured_and_clobbered_before_load_same_bb_1`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844