[PATCH] D130896: [AA] Tracking per-location ModRef info in FunctionModRefBehavior (NFCI)

[Evgeniy via Phabricator via llvm-commits]

ebrevnov accepted this revision.
ebrevnov added a comment.

I've left some NITs which are not blockers and can be addressed later. LGTM.



================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:257
+  /// Create FunctionModRefBehavior that can read and write any memory.
+  static FunctionModRefBehavior unknown() {
+    return anyLoc(ModRefInfo::ModRef);
----------------
Naming suggestion: readWrite or canReadWrite or doReadWrite.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:262
+  /// Create FunctionModRefBehavior that cannot read or write any memory.
+  static FunctionModRefBehavior none() {
+    return FunctionModRefBehavior();
----------------
Naming suggestion: noReadWrite() or dontReadWrite.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:263
+  static FunctionModRefBehavior none() {
+    return FunctionModRefBehavior();
+  }
----------------
Default constructor makes unnecessary dependence on internals of ModRefInfo. In particular, it relays on NoModRef being 0. My suggestion is to explicitly use anyLoc(ModRefInfo::NoModRef) here (default constructor can be removed then since this looks to be the only use)


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:308
+  /// Get new FunctionModRefBehavior with NoModRef on the given Loc.
+  FunctionModRefBehavior getWithoutLoc(Location Loc) const {
+    FunctionModRefBehavior FMRB = *this;
----------------
Do we really need this API? Looks like it is simply  a wrapper and equivalent to getWithModRef(Loc, ModRefInfo::NoModRef) call, which I find more readable. WDYT?



================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:352
+
+  /// Intersect (in-place) with another FunctionModRefBehavior.
+  FunctionModRefBehavior operator&(FunctionModRefBehavior Other) const {
----------------
I believe "(in-place)" should be removed since it returns new instance.


================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:363
+
+  /// Union (in-place) with another FunctionModRefBehavior.
+  FunctionModRefBehavior operator|(FunctionModRefBehavior Other) const {
----------------
I believe "(in-place)" should be removed since it returns new instance.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130896/new/

https://reviews.llvm.org/D130896