[PATCH] D132657: [DSE] Eliminate noop store even through has clobbering between LoadI and StoreI

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1879
+          if (LoadAccess != Current) {
+            if (auto *CurrentDef = cast<MemoryDef>(Current))
+              if (auto *CurrentStoreI =
----------------
Drop the if here, cast never returns nullptr.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1896
+                    ToCheck.insert(MSSA.getWalker()->getClobberingMemoryAccess(
+                        CurrentDef));
+                    continue;
----------------
This doesn't looks correct: It will look for a clobber of CurrentDef, while we are interested in clobbers of Def.

It's probably possible to show an issue using a sequence like this:

```
store i32 %v, ptr %p
store i32 %v2, ptr %p2, !alias.scope !0
store i32 %v, ptr %p, !noalias !0
store i32 %v, ptr %p2
```
With appropriate metadata to make those two stores non-aliasing.

Just using `getDefiningAccess()` should be correct. Alternatively using the MemoryLocation-based clobber API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132657