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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844