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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 10 inline comments as done.
asbirlea added a comment.

Updated, PTAL.



================
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.
----------------
sanjoy wrote:
> 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.
Should not be an issue anymore. If getArgModRefInfo returns results with MRI_Must, result should still be correct.


================
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.
----------------
sanjoy wrote:
> 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.
Ack, updated.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:506
+    if (AR == MustAlias)
+      return MRI_MustModRef;
+  }
----------------
nlopes wrote:
> nlopes wrote:
> > AtomicCmpXchgInst must read, but may not write (depending on the value read).
> > I think the only safe value here is MRI_ModRef, since the current lattice doesn't have a more precise value for MustRead & MayReadWithMustAlias.
> gah, I meant MustRef & MayModWithMustAlias.
I updated the comments above for the MRI_Must* bits. MustRef & MayModWithMustAlias should have Must bit set.


https://reviews.llvm.org/D38862