[PATCH] D103009: [DSE] Use calloc for memset+malloc

[Dawid Jurczak via Phabricator via llvm-commits]

yurai007 marked 2 inline comments as done.
yurai007 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1787
     if (StoredConstant && StoredConstant->isNullValue()) {
-      auto *DefUOInst = dyn_cast<Instruction>(DefUO);
-      if (DefUOInst && isCallocLikeFn(DefUOInst, &TLI)) {
-        auto *UnderlyingDef = cast<MemoryDef>(MSSA.getMemoryAccess(DefUOInst));
-        // If UnderlyingDef is the clobbering access of Def, no instructions
-        // between them can modify the memory location.
-        auto *ClobberDef =
-            MSSA.getSkipSelfWalker()->getClobberingMemoryAccess(Def);
-        return UnderlyingDef == ClobberDef;
+      auto *DefUOInst = const_cast<Instruction *>(dyn_cast<Instruction>(DefUO));
+      if (DefUOInst) {
----------------
nikic wrote:
> Which part here requires the const cast?
Malloc, more precisely starting from this line: IRBuilder<> IRB(Malloc);
We can const_cast later, at time of Malloc definition but we cannot remove it completely - it's still required for Builder and replaceAllUsesWith/eraseFromParent.
Anyway, I moved it to Malloc definition as it's more appropriate place.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1822
+                Malloc->replaceAllUsesWith(Calloc);
+                Malloc->eraseFromParent();
+                return true;
----------------
nikic wrote:
> Looks like this doesn't preserve MemorySSA? Try `-passes='dse,verify<memoryssa>'` in the test.
Will check.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/noop-stores.ll:340
+  ret i8* %call
+}
+
----------------
nikic wrote:
> As it was the motivating case, I'd also expect a test where the memset is in a different block.
> 
> I also don't think that the dominates() condition in your implementation is exercised by tests. Some other conditions aren'T either, for example malloc and memset having different sizes.
> I also don't think that the dominates() condition in your implementation is exercised by tests. Some other conditions aren'T either, for example malloc and memset having different sizes.

Sure, I can add much more tests to cover more conditions.
> As it was the motivating case, I'd also expect a test where the memset is in a different block.
 Currently with this patch DSE cannot perform such transformation. Consider original pr25892 from https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/InstCombine/memset-1.ll.
IIRC the reason is that malloc block - entry doesn't have related local store to malloced memory so DSE cannot find malloc-memset pair as candidate for elimination. I can provide more details if you want but when I checked it last time I simply concluded that more effort in DSE would be needed to make it work across blocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103009