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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Thanks for this, changes look reasonable to me.

IIUC, there will be a +0.30% in tramp3d with this, but without the caching it would be +0.51%?



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:906
 
+  DenseMap<const Value *, Instruction *> EarliestEscapes;
+  DenseMap<Instruction *, TinyPtrVector<const Value *>> Inst2Obj;
----------------
Does it make sense to use SmallDenseMap?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1270
 
+  bool MaybeCapturedBefore(const Value *Object, Instruction *I) {
+    if (!isa<AllocaInst>(Object))
----------------
Could you name either "MayBeCapturedBefore" or return the negated condition and rename "MayNotBeCapturedBefore"?


================
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, {}});
----------------
If this can be `nullptr` (per the return condition below), shouldn't inserting into `Inst2Obj` be conditioned on it being non null?


================
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);
----------------
Negated condition looks more intuitive to me:

```
return Iter.first->second && DT.properlyDominates(I, Iter.first->second) &&
    !isPotentiallyReachable(I->getNextNode(), I, nullptr, &DT, &LI);
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109844