[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 21 15:20:46 PST 2018
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
More information about the llvm-commits
mailing list