[PATCH] D82204: [DSE,MSSA] Treat `store 0` after calloc as noop stores.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1943
+        // between them can modify the memory location.
+        auto *ClobberDef = MSSA.getSkipSelfWalker()->getClobberingMemoryAccess(
+            Def->getDefiningAccess(), DefLoc);
----------------
asbirlea wrote:
> One small note here: this query will not cache any result, because it's checking access to the DefLoc. It seems like the scenario of having: store value 0 + calloc as underlying object may be rare enough that it will not kick in often without benefit, but it's also possible to see a compile-time regression if the following pattern is found often.
> ```
> calloc 
> store val, loc
> store 0, loc
> ```
> 
> In contrast, doing getClobbering(Def) will cache and the info could be used by this or subsequent passes.  Would it make sense to make this check here instead?
Thanks for pointing that out, I updated the code to use ` getClobberingMemoryAcess(Def)`.

 `getClobberingMemoryAccess(Access, Loc) is also used in getDomMemoryDef and profiling indicates that this is where 50%+ of the time in DSE with MemorySSA is spent. `getClobberingMemoryAcess(Def)` could be used for the check in the first iteration, but for subsequent ones we need to pass in the original location I think. Any thoughts/suggestions on how to improve things compile-time wise there would be greatly appreciated. But that's for separate changes :)


================
Comment at: llvm/test/Transforms/DeadStoreElimination/MSSA/calloc-store.ll:113
+  store i8 0, i8* %p.2, align 1
+  ret i8* %p
+}
----------------
asbirlea wrote:
> Negative test: add before ret and check it does not get eliminated:
> ```
> %p.3.2 = getelementptr i8, i8* %p, i32 3
>  store i8 0, i8* %p.3.2, align 1
> ```
Done, I've added it as a separate test. Currently only the last store is left over, but as it is a store of 0, it could also be eliminated. We currently don't do that, because of the order in which we perform the eliminations. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82204