[PATCH] D120133: [instsimplify] Assume storage for byval args doesn't overlap allocas, globals, or other byval args

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2550
   return isa<AllocaInst>(V1) &&
     (isa<AllocaInst>(V2) || isa<GlobalVariable>(V2));
 }
----------------
nikic wrote:
> nikic wrote:
> > reames wrote:
> > > nikic wrote:
> > > > Can't this function be simplified to `isByValArgOrAllocaOrGlobalVar(V1) && isByValArgOrAllocaOrGlobalVar(V2)`? Is there some reason your implementation is going out of the way to not handle the GlobalVariable + GlobalVariable case? (Note that GlobalVariable != GlobalObject, so this does not included aliases).
> > > The bit of code your commenting on was the original structure - I moved it, but didn't change it.
> > > 
> > > When I posted this, I hadn't dug into the global piece in detail.  I have now.  Global are slightly tricky because not all them are guaranteed disjoint - or at least that's what the code in constant folding seems to imply?  We don't actually get here with globals - they're constantexprs - but I didn't want to risk adding rarely executed potentially divergent behavior.
> > > 
> > > I think that your suggestion probably is okay, but I'd strongly prefer to go with the current structure for now, and restructure in a separate change for risk isolation.  My hope is to eventually pull this out in a way where we can reuse it in constant folding as well.  
> > Hm yeah, I guess the two global case is not legal for unnamed_addr globals, which may be merged.
> > 
> > I think it may still be worthwhile to write this in a way that explicitly excludes that case, rather than listing different permutations of possible combinations. Something like this, maybe?
> > 
> > ```
> > auto IsDisjointObject = [](Value *V) {
> >   auto *Arg = dyn_cast<Argument>(V);
> >   return isa<AllocaInst>(V) || isa<GlobalVariable>(V) || (Arg && Arg->hasByValAttr());
> > };
> > if (!IsDisjointObject(V1) || !IsDisjointObject(V2))
> >   return false;
> > 
> > // Two globals may have the same address.
> > // TODO: Make this specific to unnamed_addr globals.
> > return !isa<GlobalVariable>(V1) || !isa<GlobalVariable>(V2);
> > ```
> But I think it's also okay as-is...
I will play with some options here and try posting something in another review.  


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

https://reviews.llvm.org/D120133