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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

This certainly needs some test cases.



================
Comment at: include/llvm/Analysis/AliasAnalysis.h:109
+  MRI_ModRef = MRI_Ref | MRI_Mod,
+  /// The access must alias the value stored in memory.
+  MRI_Must = 4,
----------------
We should explicitly document here whether this is only for complete overlap, or whether it includes partial overlaps.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:128
     // Early-exit the moment we reach the bottom of the lattice.
-    if (Result == MRI_NoModRef)
+    if ((Result & MRI_ModRef) == MRI_NoModRef)
       return Result;
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > Probably-silly question: what's the difference between `Result == MRI_NoModRef` and `Result == MRI_Must`?
> > 
> > (If the answer is "nothing", should we ever really see `MRI_Must` on its own?)
> Not silly at all. In theory Result == MRI_Must can happen when checking ModRefInfo for 2 locations that alias each other, and both only read. 
> I updated the check for safety. I am not sure we should see MRI_Must on its own, but there is nothing preventing it right now.
Right, we currently don't consider read/read aliasing. With this change, we do. Why? (And this should be documented explicitly).


================
Comment at: lib/Analysis/AliasAnalysis.cpp:151
+    if (MR & MRI_ModRef)
+      return ModRefInfo(MRI_ModRef | (MR & MRI_Must));
   }
----------------
How is including MR's Must bit useful? What if the call has multiple pointer-typed arguments?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:529
+  ModRefInfo M = MRI_Must;
+  // FIXME: MRI_Must info is incomplete due to early return.
+  // Set flag only if no May found and all operands processed.
----------------
This still needs to be fixed?


================
Comment at: lib/Analysis/AliasAnalysisEvaluator.cpp:248
       case MRI_NoModRef:
+      case MRI_Must:
         PrintModRefResults("NoModRef", PrintNoModRef, I, Pointer,
----------------
We'll need to print the information for testing.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:780
     ModRefInfo Result = MRI_NoModRef;
+    ModRefInfo MustResult = MRI_Must;
 
----------------
Why is this useful? If we don't know which operand it is, I don't see how we use the information that it must alias one of the operands.


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


https://reviews.llvm.org/D38862