[PATCH] D38862: Add must alias info to ModRefInfo.
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 18 13:33:11 PST 2017
sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.
Mostly minor stuff. The main non-nit comment I have is that the various places we're handling calls don't seem consistent with each other.
================
Comment at: lib/Analysis/AliasAnalysis.cpp:201
+ else
+ MayAlias = true;
}
----------------
How about:
```
bool MustAlias = true;
AliasResult ArgAlias = ...
MustAlias &= ArgAlias == MustAlias;
```
?
================
Comment at: lib/Analysis/AliasAnalysis.cpp:291
+ if (isModOrRefSet(ArgMask)) {
+ MustAliasFound = true;
+ if (!isMustSet(ModRefCS1))
----------------
Given that you're setting `MustAliasFound` even if `!isMustSet(ModRefCS1)` perhaps it should be called something else?
Secondly, how about structuring this as:
```
bool MustAlias = true;
MustAias &= isMustSet(ModRefCS1);
```
?
Finally, I don't think `MustAliasFound` is necessary anymore -- if there were no MustAlias results and there was at least one argument, then we'd have cleared out `MustAlias`, so no need to track `MustAliasFound` I think?
================
Comment at: lib/Analysis/AliasAnalysis.cpp:299
+
R = intersectModRef(unionModRef(R, ArgMask), Result);
+ if (R == Result) {
----------------
Unrelated to your change, but this is the second time I've been confused by this phrase. The variable `Result` sounds like it is the value we're going to return. But it isn't -- it is just the ModRef information we've computed till before this algorithm on arguments.
What do you think about (in a separate change) splitting out this loop (and the one after) into a separate function, in which `Result` will be passed as an argument with a more mnemonic name? Of course, this is totally unrelated to your change, so I'll understand if you don't think it is a good use of your time. :)
================
Comment at: lib/Analysis/AliasAnalysis.cpp:303
+ if (I + 1 != E)
+ MayAlias = true;
break;
----------------
I think we can simplify this a bit -- just keep track of the number of arguments you've seen to be `MustAlias`, and check that it is equal to the number of actual function args. That way we're not using `MayAlias` to track "have seen a may-alias OR have early exited".
================
Comment at: lib/Analysis/AliasAnalysis.cpp:345
+ else
+ MayAlias = true;
+
----------------
The comments I made on the previous loop also apply here.
================
Comment at: lib/Analysis/AliasAnalysis.cpp:591
+ if (AR == NoAlias)
continue;
+ if (AR == MayAlias || AR == PartialAlias)
----------------
Should you be clearing `MustAlias` even if `AR == NoAlias` like you're doing in the other cases?
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:810
continue;
+ if (AR == MayAlias || AR == PartialAlias)
+ MustAlias = false;
----------------
Same comment here w.r.t. setting `MustAlias` when `AR == NoAlias`.
================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:864
// Loc is exactly the memcpy source thus disjoint from memcpy dest.
- return ModRefInfo::Ref;
+ return ModRefInfo::MustRef;
if ((DestAA = getBestAAResults().alias(MemoryLocation::getForDest(Inst),
----------------
Going by our current definition ("Must means no other location is Ref'ed or Mod'ed") this can't return Must right?
https://reviews.llvm.org/D38862
More information about the llvm-commits
mailing list