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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2550
   return isa<AllocaInst>(V1) &&
     (isa<AllocaInst>(V2) || isa<GlobalVariable>(V2));
 }
----------------
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...


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

https://reviews.llvm.org/D120133