[PATCH] D40749: Modify ModRefInfo values using static inline method abstractions [NFC].

[Sanjoy Das via Phabricator via llvm-commits]

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

Hi,

I have some minor comments inline.

I only skimmed through the parts that seemed to be mechanical transformations, but please let me know if there are specific places that should review more carefully.



================
Comment at: include/llvm/Analysis/AliasAnalysis.h:111
 };
+static inline bool isNoModRef(ModRefInfo MRI) { return MRI == MRI_NoModRef; }
+static inline bool modOrRefSet(ModRefInfo MRI) { return MRI & MRI_ModRef; }
----------------
Why `static`?


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:112-113
+static inline bool isNoModRef(ModRefInfo MRI) { return MRI == MRI_NoModRef; }
+static inline bool modOrRefSet(ModRefInfo MRI) { return MRI & MRI_ModRef; }
+static inline bool modAndRefSet(ModRefInfo MRI) {
+  return (MRI & MRI_ModRef) == MRI_ModRef;
----------------
`isModOrRef`, `isMod` etc. (alternatively `isModOrRefSet` or `isModSet`) sound more idiomatic to me.

[edit: I also have another note later on the `setXXX` naming].


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:134
+}
+static inline ModRefInfo modRefUnion(ModRefInfo MRI1, ModRefInfo MRI2) {
+  return ModRefInfo(MRI1 | MRI2);
----------------
We should stick to the `<verb><noun>` format here too, so `unionModRef` and `intersectModRef`.


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:553
       if (auto CS = ImmutableCallSite(I)) {
-        auto MRB = getModRefBehavior(CS);
-        if ((MRB & MRI_ModRef) == MRI_ModRef)
-          return MRI_ModRef;
-        if (MRB & MRI_Ref)
-          return MRI_Ref;
-        if (MRB & MRI_Mod)
-          return MRI_Mod;
-        return MRI_NoModRef;
+        return ModRefInfo(getModRefBehavior(CS));
       }
----------------
This does not seem NFC to me -- doesn't `getModRefBehavior` return a `FunctionModRefBehavior` with the extra bits set?  Or is the semantics of enum such that `ModRefInfo` will strip the unnecessary bits?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:151
+    if (modOrRefSet(MR))
+      return setModAndRef(MR);
   }
----------------
One tricky aspect here is that `setModAndRef(XX)` sounds a bit like "change `XX` in place and set that bit" when the semantics is more like `getWithModAndRefSet`, but that's a bit verbose.  What do you think?  I personally think the verbosity is worth it, but you're touching a lot of this code so I'll leave this to your intuition.

We should also at least mark these "setters" as WARN_UNUSED_VALUE since `setModAndRef(XX);` (or some other spelling) with no assignment of the result is certainly a bug.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:452
 
-  // If the cmpxchg address does not alias the location, it does not access it.
-  if (Loc.Ptr && !alias(MemoryLocation::get(CX), Loc))
-    return MRI_NoModRef;
+  if (Loc.Ptr) {
+    AliasResult AR = alias(MemoryLocation::get(CX), Loc);
----------------
Why do you need this change?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:470
+  if (Loc.Ptr) {
+    AliasResult AR = alias(MemoryLocation::get(RMW), Loc);
+    // If the atomicrmw address does not alias the location, it does not access
----------------
Same here -- this change seems unnecessary.


https://reviews.llvm.org/D40749