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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Thank you for the comments and the review! I will land this tomorrow.



================
Comment at: include/llvm/Analysis/AliasAnalysis.h:232
+// Wrapper method strips bits significant only in FunctionModRefBehavior,
+// to obtain a valid ModRefInfo. UB if those bits are not stripped out.
+// The benefit of using the wrapper is that if ModRefInfo enum changes,
----------------
sanjoy wrote:
> > UB if those bits are not stripped out.
> 
> I don't see how this is relevant here.
The comment was meant so possible future changes don't assume `ModRefInfo(FMRB) ` will drop most significant bits. Removing and hoping that doesn't happen :).


================
Comment at: lib/Analysis/AliasAnalysis.cpp:267
+        ModRefInfo ArgMask;
+        if (isModSet(ArgModRefCS2))
           ArgMask = MRI_ModRef;
----------------
sanjoy wrote:
> Shouldn't this be `isMod`?  Before it was checking that the return value of `getArgModRefInfo` was `== MRI_Mod`.
I made this change intentionally, here's why:

Before, it was checking `== MRI_Mod` and then resetting mask to `MRI_ModRef`. If result of `getArgModRefInfo` was already `MRI_ModRef`, it was left unchanged, but this doesn't stand out.

With this change, it's obvious that if the result of `getArgModRefInfo` is either `MRI_ModRef`or `MRI_Mod`, then the mask should be `MRI_ModRef`. The comment I added above also highlights this: that if CS2 mods the location, we're looking to further check if CS1 reads or writes, so mask should should be MRI_ModRef.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:521
     // escape.
-    if (isNoAlias(MemoryLocation(*CI), MemoryLocation(Object)))
+    if (AR == NoAlias)
       continue;
----------------
Undone this too.


https://reviews.llvm.org/D40749