[llvm] [ThinLTO][LowerTypeTests] Don't compute address taken set unless CFI (PR #118508)

[Teresa Johnson via llvm-commits]

https://github.com/teresajohnson created https://github.com/llvm/llvm-project/pull/118508

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.


>From 14b1fbbdbe54f09667bdc2ba34edb39f26ec4405 Mon Sep 17 00:00:00 2001
From: Teresa Johnson <tejohnson at google.com>
Date: Tue, 3 Dec 2024 07:38:52 -0800
Subject: [PATCH] [ThinLTO][LowerTypeTests] Don't compute address taken set
 unless CFI

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/lib/Transforms/IPO/LowerTypeTests.cpp | 23 +++++++++++-----------
 1 file changed, 11 insertions(+), 12 deletions(-)

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 =