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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:423
+void llvm::propagateFunctionAttrs(ModuleSummaryIndex &Index) {
+  auto addReadAttrs = [](std::vector<FunctionSummary *> &SCCNodes) {
+    bool ReadsMemory = false;
----------------
ncharlie wrote:
> tejohnson wrote:
> > tejohnson wrote:
> > > ncharlie wrote:
> > > > I just realized this isn't going to ever change any attrs.
> > > >  
> > > > I based it on the FunctionAttrs pass which does alias analysis first: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionAttrs.cpp#185
> > > > 
> > > > That can't be done here, since at this point we can only access summaries.
> > > > 
> > > > Maybe I need to include a flag in the function summary that marks if the function definitely writes memory and then propagate memory access information to any nodes that are unknown?
> > > Hmm, I think there is a fundamental issue with the way we designed this support in the index (my fault, not yours). We do invoke the FunctionAttrs pass during the per-module compile step. However, with a traditional LTO, doing so after the combined module is created will enable this pass to be much more aggressive because calls that were previously opaque (because they were defined elsewhere), can now be analyzed by AA. I think we will need to have some richer information in the index. E.g. at http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionAttrs.cpp#96 we skip calls in the same SCC when analyzing the memory accesses due to calls. In the compile step we have smaller SCCs and therefore treat more calls conservatively. Presumably we need to identify which calls caused the function to be marked as reading or writing memory, and then record whether the function otherwise read or wrote memory (based on it's non-call instructions), so that we can be more aggressive in the thin link attribute propagation.
> > > 
> > > While I think more about that why don't you remove this from the patch and make this first thin link patch just about propagating NoRecurse which is more straightforward? That way you can get it in much sooner.
> > Hacking up some proof-of-concept changes to get the per-call changes in the in-memory index based on accessing AA in the module summary index builder. Will send them to you to run with a little later today.
> > Presumably we need to identify which calls caused the function to be marked as reading or writing memory, and then record whether the function otherwise read or wrote memory (based on it's non-call instructions), so that we can be more aggressive in the thin link attribute propagation.
> 
> > Hacking up some proof-of-concept changes to get the per-call changes in the in-memory index based on accessing AA in the module summary index builder. Will send them to you to run with a little later today.
> 
> Cool - I'll look into this more closely after cleaning up this patch.
> 
> > While I think more about that why don't you remove this from the patch and make this first thin link patch just about propagating NoRecurse which is more straightforward? 
> 
> OK - I'll pull out the memory access propagation.
I hacked up something today. Realized that it isn't going to get much gain from where I indicated above (yes we'll have more in each SCC in the thin link, but really we gain by propagating up the graph of SCCs). Let me send you a draft patch that you can play with.


https://reviews.llvm.org/D36850