[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