[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:
> 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.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1286
+
+    return DT.dominates(I, Iter.first->second) && I != Iter.first->second &&
+           !isPotentiallyReachable(I->getNextNode(), I, nullptr, &DT, &LI);
----------------
nikic wrote:
> May be worth noting that the `dominates()` check is an approximation of `!isPotentiallyReachable`, presumably for compile-time reasons. Specifically, it misses this case, where the code-paths are disjoint, but non-dominating:
> 
> ```
> if (...) {
>     capture();
>     return;
> }
> I;
> ```
yes, the original motivation was to have `dominates` as a quick proxy check to skip the more expensive `isPotentiallyReachable` check at expense of precision. But in the latest version, `isPotentiallyReachable` isn't used during capture-tracking analysis and using `isPotentiallyReachable` here directly only leads to a small increase in compile-time (geomeans increase by ~0.01%). The number of removed stores increases noticeably.


================
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:
> 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.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:906
 
+  DenseMap<const Value *, Instruction *> EarliestEscapes;
+  DenseMap<Instruction *, TinyPtrVector<const Value *>> Inst2Obj;
----------------
asbirlea wrote:
> Does it make sense to use SmallDenseMap?
 I think we can have a substantial number of tracked escapes, but I am not sure what a good cut-off is for choosing SmallDenseMap vs DenseMap.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1276
+    if (Iter.second) {
+      Iter.first->second = FindEarliestCapture(Object, F, false, true, DT);
+      auto Ins = Inst2Obj.insert({Iter.first->second, {}});
----------------
asbirlea wrote:
> fhahn wrote:
> > asbirlea wrote:
> > > If this can be `nullptr` (per the return condition below), shouldn't inserting into `Inst2Obj` be conditioned on it being non null?
> > If it is `nullptr` this means there is no capturing instruction. I added an early exit below.
> I meant adding this if condition to not call insert with a nullptr key:
> ```
> if(Iter.first->second) {
>     auto Ins = Inst2Obj.insert({Iter.first->second, {}});
>      Ins.first->second.push_back(Object);
> }
> ```
> 
Ok got it, thanks! Updated the code.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1282
+    return !Iter.first->second || !DT.dominates(I, Iter.first->second) ||
+           I == Iter.first->second ||
+           isPotentiallyReachable(I->getNextNode(), I, nullptr, &DT, &LI);
----------------
asbirlea wrote:
> fhahn wrote:
> > asbirlea wrote:
> > > Negated condition looks more intuitive to me:
> > > 
> > > ```
> > > return Iter.first->second && DT.properlyDominates(I, Iter.first->second) &&
> > >     !isPotentiallyReachable(I->getNextNode(), I, nullptr, &DT, &LI);
> > > ```
> > Updated and inverted the function name.
> I believe `DT.dominates(I, Iter.first->second) && I != Iter.first->second` is congruent with `DT.properlyDominates(I, Iter.first->second)`.
The dominance check is gone now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844