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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

PTAL.



================
Comment at: lib/Analysis/AliasAnalysis.cpp:196
           AllArgsMask = unionModRef(AllArgsMask, ArgMask);
+          if (ArgAlias != MustAlias)
+            MayAlias = true;
----------------
sanjoy wrote:
> I'm not sure about this treatment of calls.  For a query between instructions like stores and loads that only touch one location the meaning of Must is pretty clear -- MustRef (say) means the store does not write to any location other than the location the load reads from.  However, generalizing this to calls seems to mean that a MustRef between a call and a load would imply that the call does not write to any location other than what the load reads.  This means we can't ignore NoAlias arguments -- a NoAlias (as opposed to Must) result should also set MayAlias to false.
Having looked at this again, it appears safe to set Must in this case, but I'm also quite ok with separating this into a separate patch.
I added the path to set MayAlias here and below getMRI(CS, CS), and these can be removed in a subsequent patch.


================
Comment at: lib/Analysis/GlobalsModRef.cpp:148
   void addModRefInfo(ModRefInfo NewMRI) {
-    Info.setInt(Info.getInt() | static_cast<int>(NewMRI));
+    Info.setInt(Info.getInt() | static_cast<int>(setMust(NewMRI)));
   }
----------------
sanjoy wrote:
> I wouldn't break the abstraction here this way.  Instead, I think it is better to have:
> 
> 
> ```
> int ModRefInfoToInteger(ModRefInfo mri) {
>   switch (clearMust(mri)) {
>   case NoModRef:
>     return 0;
>   case Mod:
>     return 1;
>   case Ref:
>     return 2;
>   case ModRef:
>     return 3;
>   }
> }
> 
> int IntegerToModRef(ModRefInfo mri) {
>   // Inverse of the above
> }
> ```
> 
> and use these to "encode" and "decode" an opaque `ModRefInfo` instance to a 2 bit integer and back.
> 
> 
> (You may even want to do this as a separate NFC change and land it independently.)
I tried this and ended up with a more confusing code, and harder to maintain.
The general idea of encoding 2 bits and using that in GlobalsModRef is tempting for me, but in practice it looks like it complicates the code unnecessarily. I can be convinced otherwise, and this can be an add-on clean-up patch in that case. 


https://reviews.llvm.org/D38862