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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked an inline comment as done.
asbirlea added a comment.

While updating the tests, I cannot see an obvious test scenario I missed.



================
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,
----------------
hfinkel wrote:
> We should explicitly document here whether this is only for complete overlap, or whether it includes partial overlaps.
Please let me know if the info I added here is reasonable.


================
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.
----------------
hfinkel wrote:
> This still needs to be fixed?
I want this to be the intended behavior, replaced with "Note"


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:780
     ModRefInfo Result = MRI_NoModRef;
+    ModRefInfo MustResult = MRI_Must;
 
----------------
hfinkel wrote:
> dberlin wrote:
> > hfinkel wrote:
> > > 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.
> > So, i gave an example earlier of how must information can be useful in the llvm-dev thread that i'll copy herre. 
> > It's true you don't know what operand it is, but you can ask for the location for each operand.
> > Because it doesn't matter what the values are, only what they alias:
> > 
> > ```
> > *a = something
> > foo(a)
> > b = *a
> > ```
> > 
> > If foo mustalias a, not only can you possibly move foo with a, you can actually possibly clone foo here, change it to be pass-by-value, and promote the argument inside of it (if you wanted to).
> > IE it's just as useful as known noalias for the memorylocation, and you have to do the same thing with the data.
> > 
> > If you make it 
> > ```
> > *a = something
> > foo (a, c)
> > b = *a
> > ```
> > 
> > You can only do anything if you know they are all must-alias or no-alias anyway.
> > 
> > So to me, this API is as bad/good for noalias as it is for mustalias.
> > 
> > It costs nothing to give MRI_Must, and while we don't currrently take advantage of such info, it *is* actually useful IMHO.
> > 
> > 
> I can certainly see this being useful if it means that all arguments must alias the given location. What's the useful thing to do here for call vs. call queries? Do we add MRI_Must only if all arguments must alias all other arguments, or is it enough if at least one argument on the LHS must alias an argument on the RHS?
> 
I'm sorry if this is a stupid question, I don't know what you mean by call queries.


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


https://reviews.llvm.org/D38862