[PATCH] D103009: [DSE] Use calloc for memset+malloc
Dawid Jurczak via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 10 04:47:30 PDT 2021
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
More information about the llvm-commits
mailing list