[PATCH] D101440: [DSE] Eliminate store after calloc (PR50143)

[Dawid Jurczak via Phabricator via llvm-commits]

yurai007 added inline comments.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:241
   if (onlyAccessesInaccessibleMem(MRB))
-    return ModRefInfo::NoModRef;
+    return isAllocLikeFn(Call, &TLI) ? ModRefInfo::Mod : ModRefInfo::NoModRef;
 
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > nikic wrote:
> > > > xbolva00 wrote:
> > > > > fhahn wrote:
> > > > > > From this workaround, it seems like how `inaccessiblememonly` is used for `calloc` does not completely match AA's interpretation. Are there other cases in the code base? I'm not sure if special casing `calloc` here is ideal. 
> > > > > > 
> > > > > > I think we should probably treat the `AA` change as a separate patch, with an AA only test as well, perhaps `llvm/test/Analysis/BasicAA/libfuncs.ll`?
> > > > > Not only calloc, all alloc (calloc, malloc, aligned_alloc) fns have inaccessiblememonly. inaccessiblememonly was introduced long time ago but this attribute was basically dead, no libfunc used it until recently.
> > > > > 
> > > > > 
> > > > > Sadly, inaccessiblememonly is somehow broken nowdays as the code assumes that such call does not write or read memory, but of course it returns memory.
> > > > > 
> > > > > This AA change can be separated patch (but this DSE needs it as a prerequisite)
> > > > > 
> > > > > 
> > > > > Another solution is removal of inaccessiblememonly from alloc fns, as it caused more harm then improvements.
> > > > Your new implementation claims that inaccessiblememonly alloc-like fns mod all locations -- if that's what you want, just drop the attribute from the functions entirely. That will have about the same effect.
> > > > 
> > > > I think inaccessiblememonly is fine for modeling malloc behavior (or do you see any issues there as well?), the problem is with calloc which combines malloc (inaccessiblememonly) with a write to the location (accessible). As @jdoerfert already pointed out on the PR, properly modelling this would require "inaccessible_or_returned_memonly".
> > > > 
> > > > While I'm somewhat open to DSE hacks, I don't think making this change on the AA layer is acceptable. I think dropping inaccessiblememonly inference for calloc() specifically would be the right thing to do in the meantime.
> > > Yeah, I will just drop it from calloc.
> > >> I think inaccessiblememonly is fine for modeling malloc behavior (or do you see any issues there as well?)
> > 
> > Just found one I think.
> > 
> > Imagine you want to change malloc+memset combo to calloc 
> > 
> >          auto *UnderlyingDef =
> >               cast<MemoryDef>(MSSA.getMemoryAccess(MallocInst));
> >           // If UnderlyingDef is the clobbering access of Def, no instructions
> >           // between them can modify the memory location.
> >           auto *ClobberDef =
> >               MSSA.getSkipSelfWalker()->getClobberingMemoryAccess(MemsetDef);
> >           return UnderlyingDef == ClobberDef;
> >         
> > This will fail.
> I am wondering whether there is other solution or should I just remove inaccessiblememonly from malloc too?
> 
> @jdoerfert @fhahn 
> 
> I tried also to use memoryIsNotModifiedBetween but 
> MemoryLocation MemLoc = MemoryLocation::get(MemsetInst)
> 
> MemLoc is None
@xbolva00: 
For MemsetInst getForDest(MemSet) gives correct MemLoc.
Then DT.dominates + memoryIsNotModifiedBetween combination do the job: https://pastebin.com/iBvpdQAh
It seems to be enough to teach DSE how eliminate malloc+memset and emit calloc (I checked couple of UTs and it works).
Removing inaccessiblememonly attribute in malloc or touching AA machinery is not needed.


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

https://reviews.llvm.org/D101440