[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:
> 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.  


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

https://reviews.llvm.org/D120133