[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/Transforms/Scalar/DeadStoreElimination.cpp:1270
 
+  bool MaybeCapturedBefore(const Value *Object, Instruction *I) {
+    if (!isa<AllocaInst>(Object))
----------------
asbirlea wrote:
> Could you name either "MayBeCapturedBefore" or return the negated condition and rename "MayNotBeCapturedBefore"?
I changed it to `notCapturedBefore` & added a doc-comment.


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


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


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1752
+            for (const Value *Obj : Iter->second)
+              EarliestEscapes.erase(Obj);
         }
----------------
asbirlea wrote:
> Remove `DeadInst` from `Inst2Obj`.
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844