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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry again for the delay on the reviews, not sure how I missed that they were ready. Feel free to ping me much more frequently if I don't respond - weekly is good!



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:662
     }
-    assert(Edges.size() && "Couldn't find any roots in index callgraph!");
+    if (Edges.empty()) {
+      // Failed to find root - return an empty node
----------------
Remove braces. Also, does this need to be part of D36311?


================
Comment at: lib/LTO/LTO.cpp:1051
 
+  propagateFunctionAttrs(ThinLTO.CombinedIndex);
   StringMap<FunctionImporter::ImportMapTy> ImportLists(
----------------
ncharlie wrote:
> I tried putting this in ThinLTOCodeGenerator::linkCombinedIndex(), but it seems like that function is only called by the thin-lto utility - it's never run by clang?
ThinLTOCodeGenerator is used by ld64 and other linkers that use the legacy LTO interface, and can be tested using llvm-lto. We should have these calls for both LTO APIs. I think earlier you had them only in ThinLTOCodeGenerator, and I asked to add them here, but they got removed from there in the process. If you can put it back then your test can add llvm-lto based testing lines as well. Unfortunately we still have these two LTO interfaces and need to keep them in sync.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:428
+
+    // Bail if we don't have a FunctionSummary to work with
+    if (!V.getSummaryList().size())
----------------
Add a TODO here to consider adding summaries for external nodes to hold function attribute information.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:436
+    // The dummy root node has guid of 0, so skip it
+    if (V.getGUID() == 0 || F->fflags().NoRecurse)
+      return false;
----------------
I assume we skip when NoRecurse is already true because it means it was already analyzed? If so, add comment to that effect.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:442
+        [&F](const FunctionSummary::EdgeTy &E) {
+          if (E.first.getGUID() == 0 || !E.first.getSummaryList().size())
+            return true; // might recurse - we can't reason about external
----------------
Isn't a node with a GUID of 0 the dummy root node (as per comment above)? Would we ever have an edge to it?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:453
+
+    if (!F->fflags().NoRecurse) {
+      F->fflags().NoRecurse = true;
----------------
Above we already returned if this flag was true, so I think this if check is unnecessary.


https://reviews.llvm.org/D36850