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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Looks really close to me, just some comments on the dummy root node detection. But note I had some comments on https://reviews.llvm.org/D36311 that need to be resolved.



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:428
+
+    if (F->getOriginalName() == 0 || F->fflags().NoRecurse)
+      return false;
----------------
Regarding getOriginalName, if it is being used to identify the dummy node, this needs a comment. Note if you change the GraphTraits of the SCC nodes to ValueInfo instead of FunctionSummary* as suggested in D36311, then this code would change accordingly anyway and you can invoke getGUID on that. In either case, needs a comment.

Actually, for the dummy node it might be cleaner to have a method on the Index that will return true if a given FunctionSummary* is the call graph root, and just not call this lambda in that case. But do you even need to do this? Is it an issue if the call graph root dummy node is handled the same way as the rest of the nodes and gets marked NoRecurse?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:439
+              cast<FunctionSummary>(E.first.getSummaryList().front().get());
+          bool calleeRecurses = !CFS->fflags().NoRecurse;
+          return calleeRecurses;
----------------
Just return this directly rather than storing in a bool


https://reviews.llvm.org/D36850