[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:
> 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? 
But for now, let’s start with that condition you already have.

Good enough for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103009