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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Thanks for the review, Sanjoy! Much appreciated!



================
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; }
----------------
sanjoy wrote:
> Why `static`?
My bad, removed.


================
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;
----------------
sanjoy wrote:
> `isModOrRef`, `isMod` etc. (alternatively `isModOrRefSet` or `isModSet`) sound more idiomatic to me.
> 
> [edit: I also have another note later on the `setXXX` naming].
Changing to `isModSet` makes sense, happy to make that change.
`isMod` is not the same, however. The goal is to check if it modifies, versus whether it *only* modifies. 
Concretely, `isModSet` will check `MRI & MRI_Mod`, versus `isMod` should check `MRI==MRI_Mod`.


================
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));
       }
----------------
sanjoy wrote:
> 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?
Should have researched this more before hand, yes, I assumed ModRefInfo would strip the unnecessary bits. While this happens to work, it's UB according to the standard.
Correct way is to mask out unnecessary bits before creating a ModRefInfo.
Replacing with a wrapper and adding comment to document this.



================
Comment at: lib/Analysis/AliasAnalysis.cpp:151
+    if (modOrRefSet(MR))
+      return setModAndRef(MR);
   }
----------------
sanjoy wrote:
> 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.
I wouldn't really mind the verbosity, but it feels so much simpler to read `setMod` than `getWithModSet`. I made arguments const and added LLVM_NODISCARD.
Thanks a lot for the suggestions!


================
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);
----------------
sanjoy wrote:
> Why do you need this change?
Not really needed in this patch. I reverse-created this patch from the one adding MRI_Must, where I need to check the alias result against multiple values. Undo-ing the change in this patch.


================
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
----------------
sanjoy wrote:
> Same here -- this change seems unnecessary.
Same as above, change undone.


https://reviews.llvm.org/D40749