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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:178
+      // Check whether there is a path from I to EarliestCapture.
+      return !isPotentiallyReachable(I, EarliestCapture, nullptr, &DT);
+    }
----------------
nikic wrote:
> fhahn wrote:
> > nikic wrote:
> > > This looks incorrect to me. Why would reachability between two captures matter? The way I'm reading this, if you have code like this...
> > > ```
> > > if (...) {
> > >     capture1();
> > > } else {
> > >     capture2();
> > > }
> > > ```
> > > ...outside a loop, then one of the captures will be ignored and we'll compute an incorrect earliest capture (which should be before the if in this case).
> > > 
> > > Possibly this check should be `DT.dominates(EarliestCapture, I)`? Though not sure it's necessary to explicitly check this at all.
> > > This looks incorrect to me. 
> > 
> > Yep this is not correct. I couldn't come up with a problematic test case with the earlier version of the patch due to the dominance checks in DSE.  The latest version has the logic flipped now, as in dominance check here and reachability check only in DSE.
> > 
> > > Possibly this check should be DT.dominates(EarliestCapture, I)? Though not sure it's necessary to explicitly check this at all.
> > 
> > yes, this should check for dominance. We could skip the check, but then we would fail to skip exploring branches early I think.
> As implemented now, I don't think the dominance check makes sense: You're calling isSafeToPrune() from captured(), which will effectively do the same check trying to determine the common dominator. With the current code structure, I'd suggest dropping it (and the whole isSafeToPrune method really).
> 
> What you probably have in mind is doing the check in shouldExplore() instead. That will cut of search early, in exchange for a dominance check for each (non-capturing) use. In the CapturesBefore tracking, not checking reachability for each use was a major compile-time improvement, but possibly the dominance check is cheap enough that skipping extra use exploration is a better tradeoff.
> 
> If you do want to move it into shouldExplore(), I think you'll need to drop your consistency assertions with PointerMayBeCapturedBefore(), as they could disagree in edge-cases, where you would not hit the use limit because some are discarded early.
Right, I removed `isSafeToPrune` alltogether. I guess we can look into using the dominance check in  `shouldExplore` as potential followup


================
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)) {
----------------
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/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844