[PATCH] D35633: [ThinLTO] Add FunctionAttr NoRecurse and ReadAttr propagation to ThinLTO

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D35633#816514, @ncharlie wrote:

> Just a note: this isn't fully completed yet. I still have to add tests and do some verification, but I wanted to make it's on the right track.


It looks like you are on the right track from the standpoint of dividing it up into pieces implemented during the compile step (building the index), the thin link (propagating), the backend (applying). I have a few comments after a quick initial perusal. But note when you have something ready for a full review, you should create a new patch that adds llvm-commits as a subscriber.



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:94
+    if (!Arg->getType()->isPtrOrPtrVectorTy())
+      return MAK_ReadNone;
+
----------------
These used to be continues, I think the change to an early return will mean that not all call site args are analyzed?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:443
+        bool noRecursiveCallsInFunction =
+            checkCalls(F, [F](FunctionSummary *S, const CalleeInfo *C) {
+              if (!S->fflags().NoRecurse ||
----------------
mehdi_amini wrote:
> Doc.
CalleeInfo unused.

But in any case, I don't think the checking here is sufficient as it will only detect self-recursion, not indirect recursion (e.g. A->B->A). For that you need to build the SCC (strongly connected components) on the call graph in the index. See for example addNoRecurseAttrs where it first checks if the SCC has more than one function (in which case there is automatically indirect recursion).

I wonder if the SCC builder in llvm can be made to work for the index call graph? See SCCIterator.h. You'd need to provide GraphTraits for the index graph.

Additionally, the memory access info needs to be propagated during the thin link. See how FunctionAttrs propagates them across the SCC. See addReadAttrs() for example. 


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:568
+    if (FS->fflags().ReadNone)
+      F.setDoesNotAccessMemory();
+    if (FS->fflags().ReadOnly)
----------------
As mentioned above, this needs to be computed on the SCC.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:413
+  SmallSetVector<Function *, 8> SCCNodes;
+  for (Function &C : TheModule.getFunctionList()) {
+    SCCNodes.insert(&C);
----------------
ncharlie wrote:
> I'm don't think this entirely correct. The reason I did it this way was so I could reuse the computeFunctionBodyMemoryAccess from the original FunctionAttrs pass. I think I'll need to modify that function a bit to work with the Index so it can access information about functions external to the current module.
This is called from the compile step, so there is no information in the Index for external functions. Here you just want to compute the per function attribute info to put into the index for the global analysis to use during the thin link.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:494
         &(getAnalysis<ModuleSummaryIndexWrapperPass>().getIndex());
+    computeThinMemoryAccess(M, *Index, LegacyAARGetter(*this));
     writeThinLTOBitcode(OS, ThinLinkOS, LegacyAARGetter(*this), M, Index);
----------------
Can this just be moved into the ModuleSummaryIndex builder (see ModuleSummaryAnalysis.cpp)? We should do all the building of the index there.


https://reviews.llvm.org/D35633