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

bryant via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 10 06:04:17 PST 2017


bryant added inline comments.


================
Comment at: test/Analysis/BasicAA/getforargument-crash.ll:13
+  ret void
+}
----------------
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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D28394





More information about the llvm-commits mailing list