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

[Sanjoy Das via Phabricator via llvm-commits]

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

I think we can break this patch up further -- how about this breakdown:

1. Remove the changes related to calls from this patch and do something conservative there.  That'll reduce the complexity of this patch a bit.  I also have a comment about our treatment of calls inline, which we can discuss on the subsequent patch that proper support for calls.  (Let me know if splitting this up will be annoying, and we can think of some other strategy.)
2. Make the refactoring I suggested inline in GlobalsModRef.cpp and rebase on top of it.

With both (1) and (2) done, this patch should be ready to go in modulo minor nits.



================
Comment at: include/llvm/Analysis/AliasAnalysis.h:114
+  /// Must is provided for completeness, but no routines will return only
+  /// Must today. We refer to Must being *set* when the most significant bit is
+  /// *cleared*. Conversely we *clear* Must info by *setting* the Must bit.
----------------
How about moving this meta-comment and the definition of `Must` to the end of the enum?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:196
           AllArgsMask = unionModRef(AllArgsMask, ArgMask);
+          if (ArgAlias != MustAlias)
+            MayAlias = true;
----------------
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.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:535
 /// instruction-ordering queries inside the BasicBlock containing \p I.
+/// Early exits in callCapturesBefore, may lead to Must not being set.
 /// FIXME: this is really just shoring-up a deficiency in alias analysis.
----------------
It may be better to instead add this as a comment on the return instruction, as "Not returning MustModRef since we've not seen all the arguments" etc.


================
Comment at: lib/Analysis/AliasAnalysis.cpp:566
+  bool MustAlias = true;
+  // Note: Must info is incomplete due to early return.
+  // Set flag only if no May found and all operands processed.
----------------
"incomplete" is a bit vague -- how about just leaving a comment on the return statement as "Not returning MustModRef since we've not seen all the arguments" or something like that?


================
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)));
   }
----------------
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.)


https://reviews.llvm.org/D38862