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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:113
+  /// For calls MRI_Must means all operands must alias.
+  /// MRI_Must may not be set for RAR acceses, result will be MRI_NoModRef,
+  /// even if the two location must alias. The reason is that getModRef's
----------------
*accesses


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:119
+  /// the cost of getModRef, but mearly add a piece of "free" information when
+  /// this is available. getArgModRefInfo never sets MRI_Must.
+  /// Checks with early return may also not set MRI_Must (e.g.
----------------
These last two sentences probably also belong on getArgModRefInfo and callCapturesBefore ?

I'd also wordsmith this a bit -- something like:

```
We usually do not try our best to infer MRI_Must, instead it is merely another piece of "free' information that is presented when available.  For instance:

 - (Your RAR example)
 - (Other examples)
```




================
Comment at: lib/Analysis/AliasAnalysis.cpp:151
+    if (MR & MRI_ModRef)
+      return ModRefInfo(MRI_ModRef | (MR & MRI_Must));
   }
----------------
Can just be `MR | MRI_ModRef`.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:226
     // Early-exit the moment we reach the bottom of the lattice.
-    if (Result == MRI_NoModRef)
+    if ((Result & MRI_ModRef) == MRI_NoModRef)
       return Result;
----------------
This pattern (here and later) looks a little weird to me -- either downstream passes don't care about distinguishing between `MRI_NoModRef` and `MRI_Must` or they do.  If the former is true then we should stop iterating early only when we see a `MRI_Must` and if the latter is true we should be returning `Result & MRI_ModRef`.  Otherwise I suspect we'll see hard to explain performance bugs where (e.g.):

 - Transform X optimizes better if AA returns `MRI_Must`
 - We know that alias analysis P returns `MRI_Must` for the case we're trying to optimize
 - But alias analysis Q prevents P's `MRI_Must` from being propagated by returning `MRI_NoModRef` (which is correct but not optimal).

What do you think?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:281
+        // ArgMask never has MRI_Must set. If ArgMask has Mod/Ref set, then
+        // alias exists. If MRI_Must *not* found in ArgR, set MayAlias.
+        if (ArgMask) {
----------------
is *not* found


================
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.
----------------
"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.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:452
+
+    // If the va_arg aliases the pointer as must alias, set MRI_Must
+    if (AR == MustAlias)
----------------
Here and elsewhere - please end sentences with a period.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:537
 /// with a smarter AA in place, this test is just wasting compile time.
 ModRefInfo AAResults::callCapturesBefore(const Instruction *I,
                                          const MemoryLocation &MemLoc,
----------------
Unrelated to your change, but the name of this function is odd.


================
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.
----------------
I think this is cleaner to track as `bool MustAlias`.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:784
     ModRefInfo Result = MRI_NoModRef;
+    ModRefInfo MustResult = MRI_Must;
 
----------------
Perhaps this is cleaner to track as a `bool IsMustAlias`?


================
Comment at: lib/Analysis/GlobalsModRef.cpp:87
   /// chosen to mix together with ModRefInfo bits.
+  /// FIXME: Overlaps with MRI_Must bit!
+  /// FunctionInfo.getModRefInfo() masks out everything except MRI_ModRef so
----------------
asbirlea wrote:
> hfinkel wrote:
> > I think that, if passes want to define their own "local" bits, we should create a well-defined way to make this happen. Define some enum, something like MRI_FirstUserBit, or similar, and use that to define the bit number here.
> I'd be happy with that approach, but not sure how to fix this particular case, since it appears to rely on that particular value, i.e. it looks like it needs that additional bit for alignment correctness.
> I didn't get this to work with updated values, perhaps I'm missing something obvious.
How many of these do we have in the worst case (in a clang LTO bootstrap, say)?  I'd be tempted to just have two words here.


================
Comment at: lib/Analysis/GlobalsModRef.cpp:136
   /// Adds new \c ModRefInfo for this function to its state.
   void addModRefInfo(ModRefInfo NewMRI) {
     Info.setInt(Info.getInt() | NewMRI);
----------------
I think it is better to `Info.setInt(Info.getInt() | (NewMRI & MRI_ModRef));`.  That way we won't get surprising behaviors where making AA smarter to infer `MRI_Must` regresses because that bit aliases (pun intended!) with `MayReadAnyGlobal`.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:696
+                      (MR & MRI_Must));
     switch (MR) {
     case MRI_NoModRef:
----------------
Nit: how about switching on `MR & MRI_ModRef`?


https://reviews.llvm.org/D38862