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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/CaptureTracking.h:68
+  // do not dominate each other, the terminator of the common dominator is
+  // chosen. If not all uses cannot be analyzed, the earliest escape is set to
+  // the first instruction in the function entry block. If \p V does not escape,
----------------
nit: cannot -> can


================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:178
+      // Check whether there is a path from I to EarliestCapture.
+      return !isPotentiallyReachable(I, EarliestCapture, nullptr, &DT);
+    }
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1271
+  /// Returns true if \p Object is not captured before or by \p I.
+  bool notCapturedBefore(const Value *Object, Instruction *I) {
+    if (!isa<AllocaInst>(Object))
----------------
notCaptureBeforeOrAt maybe? Not important for the load case, but is for calls.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1277
+    if (Iter.second) {
+      Iter.first->second = FindEarliestCapture(Object, F, false, true, DT);
+      auto Ins = Inst2Obj.insert({Iter.first->second, {}});
----------------
`/**/` for booleans


================
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);
----------------
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;
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844