[PATCH] D38862: Add must alias info to ModRefInfo.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:291
+        if (isModOrRefSet(ArgMask)) {
+          MustAliasFound = true;
+          if (!isMustSet(ModRefCS1))
----------------
sanjoy wrote:
> 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?
Also renamed `bool MustAlias` to `bool IsMustAlias` to avoid confusion with `AliasResult MustAlias`.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:299
+
         R = intersectModRef(unionModRef(R, ArgMask), Result);
+        if (R == Result) {
----------------
sanjoy wrote:
> 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. :)
Feedback noted. I may do this in a future patch :).


================
Comment at: lib/Analysis/AliasAnalysis.cpp:303
+          if (I + 1 != E)
+            MayAlias = true;
           break;
----------------
sanjoy wrote:
> 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".
I think that actually complicates the code. Either keeping track of the number of arguments seen so far, or of all must alias arguments, or setting a different variable on early exit, they add more baggage than using a bool to track "have seen a may-alias OR have early exited".
The comment should make this choice clear.


https://reviews.llvm.org/D38862