[PATCH] D42211: [ModRefInfo] Set ModRefInfo::Must for calls.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

The code itself lgtm (and sorry for the earlier braino), but it seems suspect to have MustAlias+NoAlias == MustAlias.  For loads and stores I thought the rule was that "Ptr <MustAlias> Store" means "Store does not touch any location other than Ptr".  However, this interpretation fails to justify what you're doing here -- if we have a call C = @f_argmemonly(p0, p1) and p1 mustlias p1 and noalias p0, we still can't return mustalias for the p0/C pair since @f_argmemonly(p0, p1) does touch a location other than p1 (i.e. p0).  It could be that I'm missing something basic here, like last time.



================
Comment at: lib/Analysis/AliasAnalysis.cpp:193
         if (ArgAlias != NoAlias) {
           ModRefInfo ArgMask = getArgModRefInfo(CS, ArgIdx);
           DoesAlias = true;
----------------
asbirlea wrote:
> sanjoy wrote:
> > Is this bit changing behavior?  Both before and after we look at `IsMustAlias` only when `DoesAlias` is true, and your change only possibly changes the value of `IsMustAlias` in the cases where `DoesAlias` is false.
> Let me try to reason this out loud.
> `IsMustAlias &= (ArgAlias == MustAlias);` is analogous with
> 
> ```
> if (ArgAlias != MustAlias)
>     IsMustAlias = false
> ```
> 
> Before: `IsMustAlias` is set to false when NoAlias is found.
> After patch: `IsMustAlias` is not set to false for NoAlias, because the statement moved inside the `if (ArgAlias != NoAlias)` block. `IsMustAlias` is now set to false for May/PartialAlias.
> 
> So for a list of arguments `(a=MustAlias, b=NoAlias)`, `DoesAlias` will be set to true in the for loop because of `a`. 
> Before the patch, `IsMustAlias=false` because of `b`, while after the patch `IsMustAlias=true` because `IsMustAlias &= (ArgAlias == MustAlias);` only resets the value for May/PartialAlias, not for NoAlias.
> 
> Does this makes sense?
> 
> Calls were setting Must when all arguments were MustAlias, this patch enables mix of MustAlias and NoAlias arguments.
> Updating patch description to reflect this.
> I'll update the comment on the clearing statement too.
Sorry, I must have misread the code.  This lgtm.


Repository:
  rL LLVM

https://reviews.llvm.org/D42211