[PATCH] D28394: [AliasAnalysis] Limit `MemoryLocation` retrieval to valid intrinsic arguments.

[bryant via Phabricator via llvm-commits]

bryant added inline comments.


================
Comment at: test/Analysis/BasicAA/getforargument-crash.ll:13
+  ret void
+}
----------------
hfinkel wrote:
> bryant wrote:
> > bryant wrote:
> > > hfinkel wrote:
> > > > bryant wrote:
> > > > > bryant wrote:
> > > > > > hfinkel wrote:
> > > > > > > bryant wrote:
> > > > > > > > hfinkel wrote:
> > > > > > > > > bryant wrote:
> > > > > > > > > > hfinkel wrote:
> > > > > > > > > > > Please also add a test case with a regular external function with a readnone argument, showing that it does not alias with some other load/store to that same address.
> > > > > > > > > > Attempting to construct this second test makes me doubt whether short-circuiting readnone arguments is the right fix. I've tried this IR:
> > > > > > > > > > 
> > > > > > > > > > ```
> > > > > > > > > > declare void @f(i8* readnone) argmemonly
> > > > > > > > > > 
> > > > > > > > > > define i8 @leave_readnone_untouched() {
> > > > > > > > > >   %a = alloca i8
> > > > > > > > > >   store i8 undef, i8* %a
> > > > > > > > > >   call void @f(i8* %a)
> > > > > > > > > >   %rv = load i8, i8* %a
> > > > > > > > > >   ret i8 %rv
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > With the patch, AA won't bother to examine f's argument, and conclude that `NoModRef:  Ptr: i8* %a        <->  call void @f(i8* %a)`.
> > > > > > > > > > 
> > > > > > > > > > Without the patch, AA would examine the argument and call `BasicAAResult::getModRefInfo(CS = f, ArgIdx = 0)`, which already possesses the savvy to return NoModRef on account of the readnone, and ultimately conclude the same result as before.
> > > > > > > > > > 
> > > > > > > > > > In light of this, what should the proper fix now be? It seems to me that the crash in my first test case purely results from the idiosyncrasies of the invariant intrinsics. In fact, they're the only ones in LangRef which use {}*, so it shouldn't be reproducible with any other intrinsic or function call.
> > > > > > > > > Okay; maybe I misunderstood the problem. Why would this crash previously upon trying to get the MRI on the intrinsic's argument? Is it because of the type of the pointer?
> > > > > > > > > 
> > > > > > > > The crash happens before MRI is checked. Before checking an argument's MRI, AA first checks if
> > > > > > > > 
> > > > > > > > 1. the argument is even a pointer (because if it's not, then no memory is involved; {}* passes this), and
> > > > > > > > 2. if the argument's MemoryLocation would even alias the original query location (this is where the crash happens), and finally, assuming that there might be an alias,
> > > > > > > > 3. checks the MRI by calling `AAResults::getArgModRefInfo` which eventually descends into `BasicAAResult::getModRefInfo`
> > > > > > > > 
> > > > > > > > > *Why* is it problematic to ask for a memory location of the parameter to invariant_end? I would expect that to just work.
> > > > > > > > 
> > > > > > > > I kind of see Philip's original point. What if we let step 1 pass, i.e., remove the check in MemoryLocation::getForArgument? I'm not entirely sure of the impact of doing this. 
> > > > > > > > if the argument's MemoryLocation would even alias the original query location (this is where the crash happens), and finally, assuming that there might be an alias,
> > > > > > > 
> > > > > > > Yes, but why does it crash?
> > > > > > > 
> > > > > > > 3. checks the MRI by calling AAResults::getArgModRefInfo which eventually descends into BasicAAResult::getModRefInfo
> > > > > > 
> > > > > > http://llvm-cs.pcc.me.uk/lib/Analysis/AliasAnalysis.cpp#179
> > > > > > 
> > > > > >         MemoryLocation ArgLoc = MemoryLocation::getForArgument(CS = invariant.end, ArgIdx = 0, TLI);
> > > > > > 
> > > > > > http://llvm-cs.pcc.me.uk/lib/Analysis/MemoryLocation.cpp#123
> > > > > > 
> > > > > >     case Intrinsic::invariant_end:
> > > > > >       assert(ArgIdx == 2 && "Invalid argument index");
> > > > > >       return MemoryLocation(
> > > > > >           Arg, cast<ConstantInt>(II->getArgOperand(1))->getZExtValue(), AATags);
> > > > > step 2, not 3.
> > > > Ah, okay. I'm sorry for leading you down the wrong path here. Philip is right: that assert is not right. That call needs to work for all pointer-typed arguments. We should return some kind of memory location there (just a default one is fine). We should also tag the argument as readnone so that it does not affect anything.
> > > > 
> > > So, have `getForArgument` return a `MemoryLocation(Ptr = nullptr)`? I think that might trigger an assert in the subsequent call to `alias` (here http://llvm-cs.pcc.me.uk/lib/Analysis/AliasAnalysis.cpp#180 ). Also, is it even right for getForArgument to return a null MemLoc? Also, if an additional check is inserted before the call to `alias`, then isn't this equivalent to my original patch that checks beforehand that `getForArgument` won't assert?
> > Ah, never mind.
> Yep, it should be fine to return a MemoryLocation() (which represents an unknown location).
Actually, `MemoryLocation(Ptr = nullptr)` does crash, but `MemoryLocation(Ptr = {}*, Size = 0)` doesn't and furthermore never aliases anything besides itself. So I'll add the latter?


Repository:
  rL LLVM

https://reviews.llvm.org/D28394