From 27f6a7962bec20f5b3143d3f944c991080554778 Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Fri, 17 Oct 2025 19:58:54 -0700 Subject: [PATCH 1/3] [WPD] Reduce ThinLTO link time by avoiding unnecessary summary analysis We are scanning through every single definition of a vtable across all translation units which is unnecessary in most cases. If this is a local, we want to make sure there isn't another local with the same GUID due to it having the same relative path. However, we were always scanning through every single summary in all cases. We shouldn't have locals and non-locals with the same GUID since local GUIDs are computed from "path:name" and non-locals from just "name". So once we find a non-local vtable summary meeting the other constraints we can just use it. This cut down a large thin link by around 6%, which was over half the time it spent in WPD. Note that we previously took the last conforming vtable summary, and now we use the first. This caused a test difference in one somewhat contrived test for vtables in comdats. --- llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | 10 +++++++++- .../ThinLTO/X86/devirt_external_comdat_same_guid.ll | 10 +++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp index a0f7ec6d5fae3..d5e2a3174b31b 100644 --- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp +++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp @@ -1168,7 +1168,11 @@ bool DevirtIndex::tryFindVirtualCallTargets( if (LocalFound) return false; LocalFound = true; - } + } else + // Don't expect to find a mix of locals and non-locals (due to path + // prefix for locals one should never have the same GUID as a + // non-local). + assert(!LocalFound); auto *CurVS = cast(S->getBaseObject()); if (!CurVS->vTableFuncs().empty() || // Previously clang did not attach the necessary type metadata to @@ -1184,6 +1188,10 @@ bool DevirtIndex::tryFindVirtualCallTargets( // with public LTO visibility. if (VS->getVCallVisibility() == GlobalObject::VCallVisibilityPublic) return false; + // Unless VS is a local, we don't need to keep looking through the rest + // of the summaries. + if (!LocalFound) + break; } } // There will be no VS if all copies are available_externally having no diff --git a/llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll b/llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll index 1f0737b719254..d0b8d14777f52 100644 --- a/llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll +++ b/llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll @@ -24,9 +24,9 @@ ; RUN: llvm-dis %t5.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR1 ; RUN: llvm-dis %t5.2.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR2 -; PRINT-DAG: Devirtualized call to {{.*}} (_ZN1A1nEi) +; PRINT-DAG: Devirtualized call to {{.*}} (_ZN1B1nEi) -; REMARK-DAG: single-impl: devirtualized a call to _ZN1A1nEi +; REMARK-DAG: single-impl: devirtualized a call to _ZN1B1nEi target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-grtev4-linux-gnu" @@ -55,7 +55,7 @@ entry: ret i32 0 } -; CHECK-IR1: define i32 @test( +; CHECK-IR1: define noundef i32 @test( define i32 @test(ptr %obj, i32 %a) { entry: %vtable = load ptr, ptr %obj @@ -65,7 +65,7 @@ entry: %fptr1 = load ptr, ptr %fptrptr, align 8 ; Check that the call was devirtualized. - ; CHECK-IR1: tail call i32 {{.*}}@_ZN1A1nEi + ; CHECK-IR1: tail call i32 {{.*}}@_ZN1B1nEi %call = tail call i32 %fptr1(ptr nonnull %obj, i32 %a) ret i32 %call @@ -73,7 +73,7 @@ entry: ; CHECK-IR2: define i32 @test2 ; Check that the call was devirtualized. -; CHECK-IR2: tail call i32 @_ZN1A1nEi +; CHECK-IR2: tail call i32 @_ZN1B1nEi declare i1 @llvm.type.test(ptr, metadata) declare void @llvm.assume(i1) From 0c3d82760af3d13819d9fc7916ba41854eb75497 Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Wed, 22 Oct 2025 12:25:48 -0700 Subject: [PATCH 2/3] Update on top of PR164647 and PR164530. --- .../lib/Transforms/IPO/WholeProgramDevirt.cpp | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp index d5e2a3174b31b..6909a282b3129 100644 --- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp +++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp @@ -1161,18 +1161,10 @@ bool DevirtIndex::tryFindVirtualCallTargets( // and therefore the same GUID. This can happen if there isn't enough // distinguishing path when compiling the source file. In that case we // conservatively return false early. + if (P.VTableVI.hasLocal() && P.VTableVI.getSummaryList().size() > 1) + return false; const GlobalVarSummary *VS = nullptr; - bool LocalFound = false; for (const auto &S : P.VTableVI.getSummaryList()) { - if (GlobalValue::isLocalLinkage(S->linkage())) { - if (LocalFound) - return false; - LocalFound = true; - } else - // Don't expect to find a mix of locals and non-locals (due to path - // prefix for locals one should never have the same GUID as a - // non-local). - assert(!LocalFound); auto *CurVS = cast(S->getBaseObject()); if (!CurVS->vTableFuncs().empty() || // Previously clang did not attach the necessary type metadata to @@ -1188,10 +1180,7 @@ bool DevirtIndex::tryFindVirtualCallTargets( // with public LTO visibility. if (VS->getVCallVisibility() == GlobalObject::VCallVisibilityPublic) return false; - // Unless VS is a local, we don't need to keep looking through the rest - // of the summaries. - if (!LocalFound) - break; + break; } } // There will be no VS if all copies are available_externally having no @@ -2599,6 +2588,11 @@ void DevirtIndex::run() { if (ExportSummary.typeIdCompatibleVtableMap().empty()) return; + // Assert that we haven't made any changes that would affect the hasLocal() + // flag on the GUID summary info. + assert(!ExportSummary.withInternalizeAndPromote() && + "Expect index-based WPD to run before internalization and promotion"); + DenseMap> NameByGUID; for (const auto &P : ExportSummary.typeIdCompatibleVtableMap()) { NameByGUID[GlobalValue::getGUIDAssumingExternalLinkage(P.first)].push_back( From 3cb75fd64acb6c8f6b188fed36c2b37c5e286ead Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Wed, 22 Oct 2025 16:13:06 -0700 Subject: [PATCH 3/3] Add hasLocal methods --- llvm/include/llvm/IR/ModuleSummaryIndex.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h index cdfee727387a5..eb60bee309cf5 100644 --- a/llvm/include/llvm/IR/ModuleSummaryIndex.h +++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h @@ -178,6 +178,8 @@ struct alignas(8) GlobalValueSummaryInfo { /// only be called prior to index-based internalization and promotion. inline void verifyLocal() const; + bool hasLocal() const { return HasLocal; } + private: /// List of global value summary structures for a particular value held /// in the GlobalValueMap. Requires a vector in the case of multiple @@ -239,6 +241,8 @@ struct ValueInfo { void verifyLocal() const { getRef()->second.verifyLocal(); } + bool hasLocal() const { return getRef()->second.hasLocal(); } + // Even if the index is built with GVs available, we may not have one for // summary entries synthesized for profiled indirect call targets. bool hasName() const { return !haveGVs() || getValue(); }