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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:177
+        unsigned ArgIdx = A.Index;
+        if (!Arg->getType()->isPointerTy() || CS.doesNotAccessMemory(ArgIdx))
           continue;
----------------
bryant wrote:
> reames wrote:
> > I'm missing something here.  *Why* is it problematic to ask for a memory location of the parameter to invariant_end?  I would expect that to just work.  
> Here are the type sigs for the invariant intrinsics:
> 
>     declare {}* @llvm.invariant.start.p0i8(i64 <size>, i8* nocapture <ptr>)
>     declare void @llvm.invariant.end.p0i8({}* readnone <start>, i64 <size>, i8* nocapture <ptr>)
> 
> And the semantics for invariant.start:
> 
> > This intrinsic indicates that until an ``llvm.invariant.end`` that uses the return value, the referenced memory location is constant and unchanging.
> 
> So if I were to venture a guess, the {}* opaque return is meant to be used as a token, not a real memory address. Only the <ptr> parameter of both is a legit pointer, so getForArgument is likewise restricted to that.
> 
> (As an aside: Why not have invariant.start return a real pointer to <ptr> with the desired memory semantics?)
> 
> I am definitely not an expert on this and defer to Hal for a real explanation.
Yes, I think this explanation is correct. That pointer is not really a pointer, but rather just a token value used to pair the starting and ending intrinsics. I'm not sure why it is a pointer value at all, but that seems somewhat orthogonal. We could also solve this particular problem by tagging the token return value with noalias, but I think this is more-general solution (although we could do both). In short, this is something we should be doing anyway.



================
Comment at: test/Analysis/BasicAA/getforargument-crash.ll:4
+declare {}* @llvm.invariant.start(i64, i8* nocapture) nounwind readonly
+declare void @llvm.invariant.end({}*, i64, i8* nocapture) nounwind
+
----------------
You don't have readnone on the first argument; adding it would make it more clear what's going on.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D28394