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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Some minor nits inline, but I thought we were going with making bitwise `MRI_Must` be `0` (and `MRI_NoModRef` be `4`) so that `&` works as expected?



================
Comment at: include/llvm/Analysis/AliasAnalysis.h:111
+  /// [ About MRI_Must:
+  /// MRI_Must is set in a conservative best effort manner.
+  /// We usually do not try our best to infer MRI_Must, instead it is merely
----------------
(Minor) I'd remove "conservative" -- it is understood that these things are conservatively correct.


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:115
+  /// Concrete cases:
+  /// - For calls MRI_Must means only MustAlias or NoAlias was found on all
+  /// arguments.
----------------
This is for calls that are known to be `argmemonly`, right?


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:122
+  /// set the must flag in this case may be expensive.
+  /// - getArgModRefInfo never sets MRI_Must.
+  /// - Checks with early return may also not set MRI_Must (e.g.
----------------
I think this comment can be improved.

 - Instead of starting with details on how we infer `MRI_Must` let's first describe what it means, followed by this note.
 - I don't think we need a laundry list of examples here.  I think it is better to just mention that we don't always return the most precise possible and add a "c.f. getArgModRefInfo, callCapturesBefore etc.".


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:127
+  /// We currently avoid returning MRI_Must, default is MRI_NoModRef.
+  MRI_Must = 4,
+  /// The access may reference the value stored in memory,
----------------
"default" is a bit confusing here.  How about just leaving out the second part, and commenting "MRI_Must is provided for completeness, but no routines will return only MRI_Must today".


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:654
   /// instruction ordering queries inside the BasicBlock containing \p I.
+  /// Early exits in callCapturesBefore, may lead to MRI_Must not being set.
   ModRefInfo callCapturesBefore(const Instruction *I,
----------------
The comma is unnecessary.


================
Comment at: lib/Analysis/AliasAnalysisEvaluator.cpp:261
+      case MRI_Mod:
+        PrintModRefResults("Just Mod", PrintMod, I, Pointer, F.getParent());
+        ++ModCount;
----------------
Unnecessary move?  (Not a big deal, but avoid this will make the diff slightly smaller)


https://reviews.llvm.org/D38862