[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation

[Charles Saternos via Phabricator via llvm-commits]

ncharlie added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:434
+        [&F](const FunctionSummary::EdgeTy &E) {
+          if (E.first.getGUID() == 0 || !E.first.getSummaryList().size())
+            return true; // might recurse - we can't reason about external
----------------
tejohnson wrote:
> ncharlie wrote:
> > tejohnson wrote:
> > > ncharlie wrote:
> > > > This is currently failing a test case that uses printf. Since printf doesn't have a summary associated with it (i.e. the SummaryList is empty), I can't determine if it recurses and have to fail out early.
> > > > 
> > > > Is there some spot I should add code to create a FunctionSummary for external functions?
> > > How is printf handled in the full LTO case? Does it have attributes indicating that it is no recurse? Otherwise I think treating it conservatively is the only option.
> > In FullLTO it's able to determine that printf doesn't recurse by analyzing the callsite (http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionAttrs.cpp#1068). Maybe I could modify ModuleSummaryAnalysis so it adds inserts the flags into the FunctionSummary while it still has access to the CallSite?
> So then that means something is able to mark the external function call as NoRecurse. Maybe because it is a known libcall.
> 
> The question is what would you mark in the summary during ModuleSummaryAnalysis? Specifically, if the same function containing the call to a NoRecurse external function contained another call to a different function (external or not) that is not NoRecurse, the caller's function summary flags will conservatively not say NoRecurse.
> 
> So in that case it does seem useful to have function summaries for external functions that contain this info. However, that might have effects in other places. My suggestion is to be conservative now, and we can think about making that enhancement as a follow-on. (Unless you had something different in mind - let me know as I haven't thought through this too closely for awhile)
Ok, I've added logic to skip over functions that don't have a FunctionSummary associated.


https://reviews.llvm.org/D36850