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

[Charles Saternos via Phabricator via llvm-commits]

ncharlie added inline comments.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:609
   }
+  propagateFunctionAttrs(*CombinedIndex);
   return CombinedIndex;
----------------
tejohnson wrote:
> Adding the calls to this file only affects the legacy LTO implementation used by ld64 and llvm-lto. You'll definitely want the calls also where you had them in the new LTO API in your earlier version of this patch (which is used by lld, gold and llvm-lto2).
Oops, forgot about llvm-lto2 - I'll move the calls back.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:423
+void llvm::propagateFunctionAttrs(ModuleSummaryIndex &Index) {
+  auto addReadAttrs = [](std::vector<FunctionSummary *> &SCCNodes) {
+    bool ReadsMemory = false;
----------------
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.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:459
+
+    if (F->getOriginalName() == 0 || F->fflags().NoRecurse)
+      return false;
----------------
tejohnson wrote:
> I don't think you want/need the check on getOriginalName?
I added this because my GraphTraits implementation for FunctionSummaries puts a dummy node in for external nodes with a GUID of zero (which we don't want to analyse).


https://reviews.llvm.org/D36850