[PATCH] D36987: [ThinLTO} Add modref information to call info in function summary

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for doing this. Some comments below, but I'd like someone else like Davide or Mehdi to review since I wrote some of this.

Can you add a test that checks for the new mod ref flags in the summary?

Also, one thing to consider would be to send a patch that has the NFC restructuring of the function attr analysis (without the new CSModRefMap parameter), that should be a straightforward patch by itself to review and doesn't need tests, then once that is in this can be rebased to better show the new changes required for the summary.



================
Comment at: include/llvm/Analysis/FunctionAttrsAnalysis.h:36
+MemoryAccessKind
+checkFunctionMemoryAccess(const Function &F, bool ThisBody, AAResults &AAR,
+                          const SCCNodeSet &SCCNodes,
----------------
Needs doxygen comment describing interface since this is now external.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:300
   struct FFlags {
-    unsigned ReadNone : 1;
-    unsigned ReadOnly : 1;
+    unsigned ReadNone : 1; // TODO: change to ReadNoneIgnoringCalls
+    unsigned ReadOnly : 1; // TODO: change to ReadOnlyIgnoringCalls
----------------
ncharlie wrote:
> Are we going with ReadNoneIgnoringCalls and ReadOnlyIgnoringCalls?
Mehdi suggested LocalRead*. My concern though about either my proposal or his is that after the Thin Link we will be updating these based on call edges and the thin link propagation, and it then becomes global. 

I'm thinking it might be better/clearer to keep the original names but include an additional flag like "ReadFlagsConsiderCalls" (don't love this name), that would be 0 in the per-module summaries, and later updated as the ReadOnly and ReadNone flags are propagated during the thin link. This would be more explicit and allow some error checking (should be set to 1 in the back end when we go to apply these flags!).

Then the behavior needs to be clearly documented here.


================
Comment at: lib/Analysis/FunctionAttrsAnalysis.cpp:34
+    const SCCNodeSet &SCCNodes,
+    DenseMap<const Function *, ModRefInfo> *CSModRefMap) {
+  FunctionModRefBehavior MRB = AAR.getModRefBehavior(&F);
----------------
Need to add clear documentation of new parameter (here and in header since this is no longer a local function). I.e. noting that if it is non-null, the ModRef information for called functions that might be pessimistic due to lack of whole program analysis will be recorded in the Map and *not* affect the return value. Also noting that this is used for ThinLTO which will perform global function attribute propagation during the thin link where we have information about functions defined in other modules.


================
Comment at: lib/Analysis/FunctionAttrsAnalysis.cpp:66
+      ModRefInfo *SaveModRef = nullptr;
+      if (!CS.hasOperandBundles() && CS.getCalledFunction() && CSModRefMap) {
+        auto Result =
----------------
Add comment that in the case of operand bundles we allow the return result to be affected by the calls, since we can't do better than this with global information.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:205
+  } else {
+    ReadNone = F.hasFnAttribute(Attribute::ReadNone);
+    ReadOnly = F.hasFnAttribute(Attribute::ReadOnly);
----------------
Perhaps note that these will be suitably conservative when AAR is not available.


================
Comment at: test/Bitcode/thinlto-function-summary-functionattrs.ll:5
 ; CHECK: <GLOBALVAL_SUMMARY_BLOCK
-; ensure @f is marked readnone
-; CHECK:  <PERMODULE {{.*}} op0=0 {{.*}} op3=1
----------------
Why remove these? 


https://reviews.llvm.org/D36987