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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

(We've discussed the meat of this patch offline, but I had some stray comments from earlier)



================
Comment at: lib/Analysis/AliasAnalysis.cpp:327
 
-        if (R == Result)
+          // ArgMask never has MRI_Must set. If Mod/Ref found (enter if), then
+          // alias exists. If MRI_Must *not* found in ArgR, set MayAlias.
----------------
asbirlea wrote:
> sanjoy wrote:
> > "ArgMask never has MRI_Must set." -> is that incidental, or a fundamental property?  In any case, if you're relying on it, then please add an assert.
> Right now getArgModRefInfo never sets MRI_Must.
> I tried to document this in ModRef enum and before getArgModRefInfo method declaration. Do you think I should also add an assert here (and above) ?
If your code will be incorrect when someone changes getArgModRefInfo to return results with `MRI_Must` set then you should definitely assert this.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:562
   ModRefInfo R = MRI_NoModRef;
+  ModRefInfo M = MRI_Must;
+  // Note: MRI_Must info is incomplete due to early return.
----------------
asbirlea wrote:
> sanjoy wrote:
> > I think this is cleaner to track as `bool MustAlias`.
> I find it easier to read having the logical OR on the final result: `ModRefInfo(R | M)`, and I think if we track it as bool the check `MustAlias?MRI_Must:MRI_NoModRef` either needs another variable or a more complex return statement.
> I'm inclined to keep as is, but both are ok.
> What do you think?
> 
> 
Here's a reason for why I think a `bool` is better here -- right now looking at just the return statement in isolation, `ModRefInfo(R | M)`, it isn't obvious that we're at most adding the `MRI_Must` bit to `R` (since `M` could be `MRI_Mod`, for instance).  But if we have `ModRefInfo(R | (MustAlias ? MRI_Must : MRI_NoModeRef))` then just by looking at the expression and nothing else we know that we're at most setting the `MRI_Must` bit.


https://reviews.llvm.org/D38862