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

[Charles Saternos via Phabricator via llvm-commits]

ncharlie added a comment.

Thank you for the feedback and corrections, @mehdi_amini and @tejohnson. I wasn't understanding the purpose of SCCNodes when I initially wrote the patch - thanks for clarifying :)

I'm going to work on GraphTraits for the ModuleSummaryIndex so SCCNodes can be used during the propagation step. After doing that and some cleanup I'll resubmit the patch to llvm-commits, unless anyone has any major modifications I should make to this patch.



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:94
+    if (!Arg->getType()->isPtrOrPtrVectorTy())
+      return MAK_ReadNone;
+
----------------
tejohnson wrote:
> These used to be continues, I think the change to an early return will mean that not all call site args are analyzed?
It's kinda messed up by the diff, I moved callsite analysis to its own function (analyseCallSite). Line 148 shows where it's called.


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

Ah, good point. Now I see what the SCCNodes are for.

> 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.

I started looking into this, and I think it'll be possible with a few more iterator types.





https://reviews.llvm.org/D35633