[PATCH] D103009: [DSE] Transform memset + malloc --> calloc (PR25892)

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1843
+            return false;
+          if (Malloc->getOperand(0) == MemSet->getLength()) {
+            if (DT.dominates(DefUOInst, MemSet) &&
----------------
xbolva00 wrote:
> nikic wrote:
> > yurai007 wrote:
> > > xbolva00 wrote:
> > > > We dont need this check, do we?
> > > > 
> > > > p = malloc(20)
> > > > memset(p, 0, 10)
> > > > 
> > > > Reading p between 10 and 20 is UB, so with calloc we would have 0s in this area so fine.
> > > > 
> > > > And reverse case is UB too.
> > > If we permitted to "calloc more than we memset" wouldn't we hide UB in some cases?
> > > Like if we would really read unitinitialized memory much later after memset?
> > > The second thing is that GCC doesn't transform malloc to calloc when we memset less memory than malloc allocates: https://godbolt.org/z/Ef94je4KP I'm not saying we should blindly follow them, I'm just not sure what was rationale behind that.
> > If the memset size is different, profitability is also unclear. Converting a malloc into a calloc may be always legal, but if you have malloc(10000) followed by memset(10) it's probably not profitable to actually do it.
> There is no rule that compiler cannot “hide” UB. Compiler is free to assume that UB never happens.
Yeah. 

Maybe if we know both sizes and memset is unlikely to be expanded (there is some limit defined samowhere), we should still prefer calloc (1 call) than 2 libcalls? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103009