[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:226
     // Early-exit the moment we reach the bottom of the lattice.
-    if (Result == MRI_NoModRef)
+    if ((Result & MRI_ModRef) == MRI_NoModRef)
       return Result;
----------------
sanjoy wrote:
> 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?
Downstream passes shouldn't rely on having `MRI_Must` set, since this bit is best effort. Yes, you could have different optimizations if that particular optimization relies on AA setting `MRI_Must`. What you should *not* have, is a performance penalty to set `MRI_Must`.

We stop iterating regardless of `MRI_Must`, because we do not want to incur penalty from calling other AA passes that may set `MRI_Must`.
And conversely, we do not return `Result & MRI_ModRef`, because if `MRI_Must` is set, the additional 'free' info should be kept.

I also updated comments in getModRef(2 callsites) hoping to clarify this.

If later on we see that `MRI_Must` is so useful that we want to fully rely on it for optimizations, then I'm happy with changing this check. Right now it's not used, so the cost of stopping only after `MRI_Must` is set is not justified.

Please let me know if this makes sense.


================
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:
> "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) ?


================
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,
----------------
sanjoy wrote:
> Unrelated to your change, but the name of this function is odd.
Ack, not planning to change in this patch though.


================
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:
> 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?




================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:784
     ModRefInfo Result = MRI_NoModRef;
+    ModRefInfo MustResult = MRI_Must;
 
----------------
sanjoy wrote:
> Perhaps this is cleaner to track as a `bool IsMustAlias`?
Same as above. Whatever we decide on that, I will apply to this one too.


================
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
----------------
sanjoy wrote:
> 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.
I'm confused. If `MayReadAnyGlobal = 2`, it will trigger the static assert below for overlapping with MRI_ModRef.


https://reviews.llvm.org/D38862