Skip to content

[WPD] Reduce ThinLTO link time by avoiding unnecessary summary analysis #164046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 23, 2025
+17 −11
Merged

[WPD] Reduce ThinLTO link time by avoiding unnecessary summary analysis #164046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27f6a79
[WPD] Reduce ThinLTO link time by avoiding unnecessary summary analysis
teresajohnson Oct 18, 2025
0c3d827
Update on top of PR164647 and PR164530.
teresajohnson Oct 22, 2025
3cb75fd
Add hasLocal methods
teresajohnson Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/include/llvm/IR/ModuleSummaryIndex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(); }
Expand Down
14 changes: 8 additions & 6 deletions llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,14 +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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit pick on the compile error: I think ValueInfo struct needs to have a method to expose hasLocal state, something like

struct ValueInfo {
  ... 
  bool hasLocal() const {
    return getRef().second.hasLocal();
  }
  ...
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have that locally, but the branch seems hopelessly messed up right now despite force pushing etc. I may have to close this PR and start a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I think my latest force push added them

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;
}
auto *CurVS = cast<GlobalVarSummary>(S->getBaseObject());
if (!CurVS->vTableFuncs().empty() ||
// Previously clang did not attach the necessary type metadata to
Expand All @@ -1184,6 +1180,7 @@ bool DevirtIndex::tryFindVirtualCallTargets(
// with public LTO visibility.
if (VS->getVCallVisibility() == GlobalObject::VCallVisibilityPublic)
return false;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, loop iterates all VS and returns false if any VS has public visibility.

After this change, the visibility check happens on the first VS. Is my understanding correct that we assumes one-definition-rule and all vtables with the same GUID has the same visibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, per https://clang.llvm.org/docs/LTOVisibility.html: "A class’s LTO visibility is treated as an ODR-relevant property of its definition, so it must be consistent between translation units."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer. The optimization on finding representative vtable value summary LGTM.

}
}
// There will be no VS if all copies are available_externally having no
Expand Down Expand Up @@ -2591,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<GlobalValue::GUID, std::vector<StringRef>> NameByGUID;
for (const auto &P : ExportSummary.typeIdCompatibleVtableMap()) {
NameByGUID[GlobalValue::getGUIDAssumingExternalLinkage(P.first)].push_back(
Expand Down
10 changes: 5 additions & 5 deletions llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -65,15 +65,15 @@ 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
}

; 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)
Expand Down