[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));
 }
----------------
reames wrote:
> 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.  
I ended up landing basically exactly your suggestion with a comment explaining the assumption we make about globals here, and we expect not to see the two globals case.  See 3a6be12.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120133