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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


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


================
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
----------------
nikic wrote:
> 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.
We should be able to use `ReadUO` , once we loaded the object the escapes cannot change it. 

> then we might be able to sink this new functionality into AA proper (e.g. via a BatchAA flag for more expensive capture analysis)

Sounds good. Do you think it would be worth to also move the caching of the escapes to BatchAA? Are there any users of BatchAA you would be interested in (other than DSE)? 


================
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(
----------------
nikic wrote:
> Isn't this test case the same as `test_captured_and_clobbered_before_load_same_bb_1`?
yes, it should use `escape_writeonly` instead of `escape_and_clobber` and call it before the first load. I'll fix that in the committed version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844