[ThinLTO][LowerTypeTests] Don't compute address taken set unless CFI (NFC) #118508
Conversation
The AddressTaken set used for CFI with regular LTO was being computed on the ExportSummary regardless of whether any CFI metadata existed. In the case of ThinLTO, the ExportSummary is the global summary index for the target, and the lack of guard in this code meant this was being computed on the ThinLTO index even when there was an empty regular LTO module, since the backend is called on the combined module to generate the expected output file (normally this is trivial as there is no IR). Move the computation of the AddressTaken set into the condition checking for CFI to avoid this overhead. This change resulted in a 20% speedup in the thin link of a large target. It looks like the outer loop has existed here for several years, but likely became a larger overhead after the inner loop was added very recently in PR113987. I will send a separate patch to refactor the ThinLTO backend handling to avoid invoking the opt pipeline if the module is empty, in case there are other summary-based analyses in some of the passes now or in the future. This change is still desireable as by default regular LTO modules contain summaries, or we can have split thin and regular LTO modules, and if they don't involve CFI these would still unnecessarily compute the AddressTaken set.
|
@llvm/pr-subscribers-llvm-transforms Author: Teresa Johnson (teresajohnson) ChangesThe AddressTaken set used for CFI with regular LTO was being computed on Move the computation of the AddressTaken set into the condition checking I will send a separate patch to refactor the ThinLTO backend handling to Full diff: https://github.com/llvm/llvm-project/pull/118508.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index 87d2432803062b..e3caefe70311b5 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -2090,20 +2090,19 @@ bool LowerTypeTestsModule::lower() {
};
MapVector<StringRef, ExportedFunctionInfo> ExportedFunctions;
if (ExportSummary) {
- // A set of all functions that are address taken by a live global object.
- DenseSet<GlobalValue::GUID> AddressTaken;
- for (auto &I : *ExportSummary)
- for (auto &GVS : I.second.SummaryList)
- if (GVS->isLive())
- for (const auto &Ref : GVS->refs()) {
- AddressTaken.insert(Ref.getGUID());
- for (auto &RefGVS : Ref.getSummaryList())
- if (auto Alias = dyn_cast<AliasSummary>(RefGVS.get()))
- AddressTaken.insert(Alias->getAliaseeGUID());
- }
-
NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions");
if (CfiFunctionsMD) {
+ // A set of all functions that are address taken by a live global object.
+ DenseSet<GlobalValue::GUID> AddressTaken;
+ for (auto &I : *ExportSummary)
+ for (auto &GVS : I.second.SummaryList)
+ if (GVS->isLive())
+ for (const auto &Ref : GVS->refs()) {
+ AddressTaken.insert(Ref.getGUID());
+ for (auto &RefGVS : Ref.getSummaryList())
+ if (auto Alias = dyn_cast<AliasSummary>(RefGVS.get()))
+ AddressTaken.insert(Alias->getAliaseeGUID());
+ }
for (auto *FuncMD : CfiFunctionsMD->operands()) {
assert(FuncMD->getNumOperands() >= 2);
StringRef FunctionName =
|
teresajohnson
changed the title
3 participants
The AddressTaken set used for CFI with regular LTO was being computed on
the ExportSummary regardless of whether any CFI metadata existed. In the
case of ThinLTO, the ExportSummary is the global summary index for the
target, and the lack of guard in this code meant this was being computed
on the ThinLTO index even when there was an empty regular LTO module,
since the backend is called on the combined module to generate the
expected output file (normally this is trivial as there is no IR).
Move the computation of the AddressTaken set into the condition checking
for CFI to avoid this overhead. This change resulted in a 20% speedup in
the thin link of a large target. It looks like the outer loop has
existed here for several years, but likely became a larger overhead
after the inner loop was added very recently in PR113987.
I will send a separate patch to refactor the ThinLTO backend handling to
avoid invoking the opt pipeline if the module is empty, in case there
are other summary-based analyses in some of the passes now or in the
future. This change is still desireable as by default regular LTO
modules contain summaries, or we can have split thin and regular LTO
modules, and if they don't involve CFI these would still unnecessarily
compute the AddressTaken set.