[PATCH] D79391: [DSE] Remove noop stores in MSSA.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1843
+
+      bool IsDead = LoadAccess == KillingDef->getDefiningAccess();
+      if (!IsDead) {
----------------
I don't entirely understand the logic here, it would help a lot to add some comments to elaborate.


What happens if `IsDead` is true? Seems this would be a good case to eliminate the store if the load/store are from/to the same address.

Next, check the condition `Load->getPointerOperand() != SI->getOperand(1)` first, and exit early if the equality condition is not met.

I see this roughly as:
```
if (Load->getPointerOperand() == SI->getOperand(1)) {
  if (LoadAccess == KillingDef->getDefiningAccess()) {
    State.deleteDeadInstruction(SI);
  } else {
    //Note: LoadAccess may not be optimized, if it is the walker call is a no-op.
    auto *LoadAccessOptimized = MSSAWalker->getClobberingMemoryAccess(MSSA.getMemoryAccess(Load));
    if (MSSAWalker->getClobberingMemoryAccess(KillingDef) == LoadAccessOptimized) {
      State.deleteDeadInstruction(SI);
    }
  }
}
```

Please let me know if I misunderstood the intention here.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1849
+        ToCheck.insert(Current);
+        for (unsigned I = 1; I < ToCheck.size(); ++I) {
+          auto *Current = ToCheck[I];
----------------
Why is this inserting two accesses then starting the checks from 1?


Updates above comment with rough replacement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79391