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

[Dawid Jurczak via Phabricator via llvm-commits]

yurai007 added inline comments.


================
Comment at: llvm/test/Transforms/DeadStoreElimination/noop-stores.ll:340
+  ret i8* %call
+}
+
----------------
yurai007 wrote:
> 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.
> Currently with this patch DSE cannot perform such transformation.(...)
After adding original PR unit test I noticed actually now DSE can perform transformation across blocks. When I checked it before it couldn't, apparently meantime changes unlocked it. Well, that's good :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103009