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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:512
+  else
+    MemLoc = MemoryLocation::get(SecondI);
   auto *MemLocPtr = const_cast<Value *>(MemLoc.Ptr);
----------------
I'm a bit surprised that MemoryLocation::get() doesn't work for MemSetInst, as it only has a single memory location, so there's no ambiguity here. @asbirlea @fhahn Do you think it would make sense to adjust the API?


================
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) {
----------------
Which part here requires the const cast?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1811
+          if (Malloc->getOperand(0) == MemSet->getLength()) {
+            if (DT.dominates(DefUOInst, cast<Instruction>(MemSet)) &&
+                memoryIsNotModifiedBetween(Malloc, MemSet, BatchAA, DL, &DT)) {
----------------
Is this cast needed? Shouldn't it be an implicit upcast?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1816
+              auto *SizeType =
+                  DL.getIntPtrType(IRB.GetInsertBlock()->getContext());
+              AttributeList EmptyList;
----------------
As you already have an IRBuilder, why not `IRB.getIntPtrTy(DL)`?


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


================
Comment at: llvm/test/Transforms/DeadStoreElimination/noop-stores.ll:340
+  ret i8* %call
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103009