This is an archive of the discontinued LLVM Phabricator instance.

[ModRefInfo] Set ModRefInfo::Must for calls.
Needs ReviewPublic

Authored by asbirlea on Jan 17 2018, 4:12 PM.
This revision needs review, but all reviewers have resigned.

Details

Reviewers
sanjoy
Summary

Follow-up patch to introducing Must in ModRefInfo, set the Must flag for a larger subset of calls.
Must is set if MustAlias and NoAlias is found (no May or Partial alias).

Diff Detail

Event Timeline

asbirlea created this revision.Jan 17 2018, 4:12 PM
sanjoy added inline comments.Jan 19 2018, 10:26 AM
lib/Analysis/AliasAnalysis.cpp
193

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.

asbirlea edited the summary of this revision. (Show Details)Jan 22 2018, 3:49 AM
asbirlea added inline comments.
lib/Analysis/AliasAnalysis.cpp
193

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.

asbirlea updated this revision to Diff 130872.Jan 22 2018, 5:59 AM

Update comments and summary.

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.

lib/Analysis/AliasAnalysis.cpp
193

Sorry, I must have misread the code. This lgtm.

Let me start of with saying, I'm not sure this is the right thing to do, you're not missing anything (and sorry for the delayed reply).
The reason I say this, is that I've had the argument with myself both ways, and both make sense depending on how we end up using this Must bit information.

Taking your example, if we have a call C = @f_argmemonly(p0, p1) and p1 mustalias p1 and noalias p0, yes, @f_argmemonly(p0, p1) does touch p0.
If we look at something like promotion, p1 should be ok to be promoted to a scalar based on finding MustAlias+NoAlias with p0. I believe this was the original reason for this patch.
If we look at something like hoisting the call, then we need a getModRef result on all function arguments. This could potentially be an additional API call.

For 2 calls, there's another discussion. Before patch: Must set means "there is a single location both calls touch, and the calls touch no other locations."
After this patch: Must set means "there exists at least one memory location that is touched by both calls, and both calls may each touch a set of disjoint memory locations".
I'm not sure how to use either info off the top of my head, so can't make the argument for/against here..

sanjoy resigned from this revision.Jan 29 2022, 5:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2022, 5:32 PM