Skip to content

[LTO][WPD] Suppress WPD on a class if the LTO unit doesn't have the prevailing definition of this class or any of its superclass #131721

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
mingmingl-llvm merged 5 commits into from
Mar 20, 2025
Merged

[LTO][WPD] Suppress WPD on a class if the LTO unit doesn't have the prevailing definition of this class or any of its superclass #131721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
40f913c
Propagate ExportDynamic from base type to compatible derived types in
mingmingl-llvm Mar 18, 2025
6be4464
implement the fix for hybrid LTO, and update test cases
mingmingl-llvm Mar 18, 2025
dfe5970
Look at the Prevailing bit as opposed to ExportDynamic bit, update
mingmingl-llvm Mar 19, 2025
27b1e88
add test coverage for regular LTO
mingmingl-llvm Mar 19, 2025
f12edd1
resolve comments
mingmingl-llvm Mar 20, 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
5 changes: 3 additions & 2 deletions llvm/lib/LTO/LTO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,7 +1314,8 @@ Error LTO::runRegularLTO(AddStreamFn AddStream) {
// expected to be handled separately.
auto IsVisibleToRegularObj = [&](StringRef name) {
auto It = GlobalResolutions->find(name);
return (It == GlobalResolutions->end() || It->second.VisibleOutsideSummary);
return (It == GlobalResolutions->end() ||
It->second.VisibleOutsideSummary || !It->second.Prevailing);
};

// If allowed, upgrade public vcall visibility metadata to linkage unit
Expand Down Expand Up @@ -1905,7 +1906,7 @@ Error LTO::runThinLTO(AddStreamFn AddStream, FileCache Cache,
auto IsVisibleToRegularObj = [&](StringRef name) {
auto It = GlobalResolutions->find(name);
return (It == GlobalResolutions->end() ||
It->second.VisibleOutsideSummary);
It->second.VisibleOutsideSummary || !It->second.Prevailing);
};

getVisibleToRegularObjVtableGUIDs(ThinLTO.CombinedIndex,
Expand Down
158 changes: 158 additions & 0 deletions llvm/test/ThinLTO/X86/devirt_prevailing.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
; RUN: rm -rf %t && mkdir %t && cd %t

; Tests that devirtualization is suppressed on a class when the LTO unit doesn't
; have the prevailing definition of the base class.

; Generate unsplit module with summary for ThinLTO index-based WPD.
; RUN: opt -thinlto-bc -o summary.o %s

; Index based WPD
; The callsite inside @_ZN4Base8dispatchEv gets devirtualized when symbol
; resolution shows there is a prevailing definition of `_ZTI7Derived` in the
; LTO unit.
; RUN: llvm-lto2 run summary.o -save-temps -pass-remarks=. \
; RUN: -thinlto-threads=1 \
; RUN: -o tmp \
; RUN: --whole-program-visibility-enabled-in-lto=true \
; RUN: --validate-all-vtables-have-type-infos=true \
; RUN: --all-vtables-have-type-infos=true \
; RUN: -r=summary.o,_ZN4Base8dispatchEv,px \
; RUN: -r=summary.o,_ZN8DerivedN5printEv,px \
; RUN: -r=summary.o,_ZTV8DerivedN,p \
; RUN: -r=summary.o,_ZTI8DerivedN,p \
; RUN: -r=summary.o,_ZTS8DerivedN,p \
; RUN: -r=summary.o,_ZTI7Derived,p \
; RUN: 2>&1 | FileCheck --allow-empty %s --check-prefix=REMARK


; Index based WPD
; The callsite inside @_ZN4Base8dispatchEv remains indirect and not de-virtualized
; when symbol resolution shows there isn't a prevailing definition of
; `_ZTI7Derived` in the LTO unit.
; RUN: llvm-lto2 run summary.o -save-temps -pass-remarks=. \
; RUN: -thinlto-threads=1 \
; RUN: -o tmp \
; RUN: --whole-program-visibility-enabled-in-lto=true \
; RUN: --validate-all-vtables-have-type-infos=true \
; RUN: --all-vtables-have-type-infos=true \
; RUN: -r=summary.o,_ZN4Base8dispatchEv,px \
; RUN: -r=summary.o,_ZN8DerivedN5printEv,px \
; RUN: -r=summary.o,_ZTV8DerivedN,p \
; RUN: -r=summary.o,_ZTI8DerivedN,p \
; RUN: -r=summary.o,_ZTS8DerivedN,p \
; RUN: -r=summary.o,_ZTI7Derived, \
; RUN: 2>&1 | FileCheck %s --allow-empty --implicit-check-not='single-impl: devirtualized a call to'

; Repeat the above tests for WPD in hybrid LTO.
; RUN: opt --thinlto-bc --thinlto-split-lto-unit -o hybrid.o %s

; RUN: llvm-lto2 run hybrid.o -save-temps -pass-remarks=. \
; RUN: -thinlto-threads=1 \
; RUN: -o hybrid-tmp \
; RUN: --whole-program-visibility-enabled-in-lto=true \
; RUN: --validate-all-vtables-have-type-infos=true \
; RUN: --all-vtables-have-type-infos=true \
; RUN: -r=hybrid.o,_ZN4Base8dispatchEv,px \
; RUN: -r=hybrid.o,_ZN8DerivedN5printEv,px \
; RUN: -r=hybrid.o,_ZTV8DerivedN, \
; RUN: -r=hybrid.o,_ZTI8DerivedN, \
; RUN: -r=hybrid.o,_ZTS8DerivedN,p \
; RUN: -r=hybrid.o,_ZTI7Derived,p \
; RUN: -r=hybrid.o,_ZN8DerivedN5printEv, \
; RUN: -r=hybrid.o,_ZTV8DerivedN,p \
; RUN: -r=hybrid.o,_ZTI8DerivedN,p \
; RUN: 2>&1 | FileCheck --allow-empty %s --check-prefix=REMARK


; RUN: llvm-lto2 run hybrid.o -save-temps -pass-remarks=. \
; RUN: -thinlto-threads=1 \
; RUN: -o hybrid-tmp \
; RUN: --whole-program-visibility-enabled-in-lto=true \
; RUN: --validate-all-vtables-have-type-infos=true \
; RUN: --all-vtables-have-type-infos=true \
; RUN: -r=hybrid.o,_ZN4Base8dispatchEv,px \
; RUN: -r=hybrid.o,_ZN8DerivedN5printEv,px \
; RUN: -r=hybrid.o,_ZTV8DerivedN, \
; RUN: -r=hybrid.o,_ZTI8DerivedN, \
; RUN: -r=hybrid.o,_ZTS8DerivedN,p \
; RUN: -r=hybrid.o,_ZTI7Derived, \
; RUN: -r=hybrid.o,_ZN8DerivedN5printEv, \
; RUN: -r=hybrid.o,_ZTV8DerivedN,p \
; RUN: -r=hybrid.o,_ZTI8DerivedN,p \
; RUN: 2>&1 | FileCheck --allow-empty %s --implicit-check-not='single-impl: devirtualized a call to'


; Repeat the above tests for WPD in regular LTO.
; RUN: opt -module-summary -o regular.o %s

; RUN: llvm-lto2 run regular.o -save-temps -pass-remarks=. \
; RUN: -thinlto-threads=1 \
; RUN: -o regular-temp \
; RUN: --whole-program-visibility-enabled-in-lto=true \
; RUN: --validate-all-vtables-have-type-infos=true \
; RUN: --all-vtables-have-type-infos=true \
; RUN: -r=regular.o,_ZN4Base8dispatchEv,px \
; RUN: -r=regular.o,_ZN8DerivedN5printEv,px \
; RUN: -r=regular.o,_ZTS8DerivedN,p \
; RUN: -r=regular.o,_ZTI7Derived,p \
; RUN: -r=regular.o,_ZTV8DerivedN,p \
; RUN: -r=regular.o,_ZTI8DerivedN,p \
; RUN: 2>&1 | FileCheck --allow-empty %s --check-prefix=REMARK

; RUN: llvm-lto2 run regular.o -save-temps -pass-remarks=. \
; RUN: -thinlto-threads=1 \
; RUN: -o regular-temp \
; RUN: --whole-program-visibility-enabled-in-lto=true \
; RUN: --validate-all-vtables-have-type-infos=true \
; RUN: --all-vtables-have-type-infos=true \
; RUN: -r=regular.o,_ZN4Base8dispatchEv,px \
; RUN: -r=regular.o,_ZN8DerivedN5printEv,px \
; RUN: -r=regular.o,_ZTS8DerivedN,p \
; RUN: -r=regular.o,_ZTI7Derived, \
; RUN: -r=regular.o,_ZTV8DerivedN,p \
; RUN: -r=regular.o,_ZTI8DerivedN,p \
; RUN: 2>&1 | FileCheck --allow-empty %s --implicit-check-not='single-impl: devirtualized a call to'

; REMARK: single-impl: devirtualized a call to _ZN8DerivedN5printEv

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@_ZTV8DerivedN = linkonce_odr hidden constant { [3 x ptr] } { [3 x ptr] [ptr null, ptr @_ZTI8DerivedN, ptr @_ZN8DerivedN5printEv] }, !type !0, !type !1, !type !2, !type !3, !vcall_visibility !4
@_ZTI8DerivedN = linkonce_odr hidden constant { ptr, ptr, ptr } { ptr null, ptr @_ZTS8DerivedN, ptr @_ZTI7Derived }
@_ZTS8DerivedN = linkonce_odr hidden constant [10 x i8] c"8DerivedN\00", align 1
@_ZTI7Derived = external constant ptr

; Whole program devirtualization will ignore summaries that are not live.
; Mark '_ZTV8DerivedN' as used so it remains live.
@llvm.used = appending global [1 x ptr] [ptr @_ZTV8DerivedN], section "llvm.metadata"

define hidden void @_ZN4Base8dispatchEv(ptr %this) {
entry:
%this.addr = alloca ptr
store ptr %this, ptr %this.addr
%this1 = load ptr, ptr %this.addr
%vtable = load ptr, ptr %this1
%0 = call i1 @llvm.type.test(ptr %vtable, metadata !"_ZTS7Derived")
call void @llvm.assume(i1 %0)
%vfn = getelementptr inbounds ptr, ptr %vtable, i64 0
%1 = load ptr, ptr %vfn
call void %1(ptr %this1)
ret void
}

define linkonce_odr hidden void @_ZN8DerivedN5printEv(ptr %this) #0 {
entry:
ret void
}

attributes #0 = { noinline optnone }

declare i1 @llvm.type.test(ptr, metadata)
declare void @llvm.assume(i1)

!0 = !{i64 16, !"_ZTS7Derived"}
!1 = !{i64 16, !"_ZTSM7DerivedFvvE.virtual"}
!2 = !{i64 16, !"_ZTS8DerivedN"}
!3 = !{i64 16, !"_ZTSM8DerivedNFvvE.virtual"}
!4 = !{i64 0}
19 changes: 19 additions & 0 deletions llvm/tools/llvm-lto2/llvm-lto2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,18 @@ static cl::opt<bool> EnableFreestanding(
cl::desc("Enable Freestanding (disable builtins / TLI) during LTO"),
cl::Hidden);

static cl::opt<bool> WholeProgramVisibilityEnabledInLTO(
"whole-program-visibility-enabled-in-lto",
cl::desc("Enable whole program visibility during LTO"), cl::Hidden);

static cl::opt<bool> ValidateAllVtablesHaveTypeInfos(
"validate-all-vtables-have-type-infos",
cl::desc("Validate that all vtables have type infos in LTO"), cl::Hidden);

static cl::opt<bool>
AllVtablesHaveTypeInfos("all-vtables-have-type-infos", cl::Hidden,
cl::desc("All vtables have type infos"));

extern cl::opt<cl::boolOrDefault> LoadBitcodeIntoNewDbgInfoFormat;
extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;

Expand Down Expand Up @@ -332,6 +344,13 @@ static int run(int argc, char **argv) {
Conf.PTO.LoopVectorization = Conf.OptLevel > 1;
Conf.PTO.SLPVectorization = Conf.OptLevel > 1;

if (WholeProgramVisibilityEnabledInLTO.getNumOccurrences() > 0)
Conf.HasWholeProgramVisibility = WholeProgramVisibilityEnabledInLTO;
Copy link
Contributor

@teresajohnson teresajohnson Mar 20, 2025

Choose a reason for hiding this comment

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

I think the test can just use the existing -whole-program-visibility flag (defined in WholeProgramDevirt.cpp) which is OR'ed with this config value, instead of adding a new llvm-lto2 flag.

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Mar 20, 2025

Choose a reason for hiding this comment

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

Yeah I spent some time looking into reusing -whole-program-visibility flag before introducing new options in llvm-lto2, but -whole-program-visibility itself doesn't initialize the VisibleToRegularObjSymbols set, which are used to skip vtable visibility upgrade.

Basically, the condition (

llvm-project/llvm/lib/LTO/LTO.cpp

Lines 1901 to 1902 in d0d33d2

if (WholeProgramVisibilityEnabledInLTO &&
Conf.ValidateAllVtablesHaveTypeInfos) {
) needs both WholeProgramVisibilityEnabledInLTO and Conf.ValidateAllVtablesHaveTypeInfos to get DenseSet<GlobalValue::GUID> VisibleToRegularObjSymbols, and WholeProgramVisibilityEnabledInLTO value is based on configuration value (

llvm-project/llvm/lib/LTO/LTO.cpp

Lines 1889 to 1893 in d0d33d2

bool WholeProgramVisibilityEnabledInLTO =
Conf.HasWholeProgramVisibility &&
// If validation is enabled, upgrade visibility only when all vtables
// have typeinfos.
(!Conf.ValidateAllVtablesHaveTypeInfos || Conf.AllVtablesHaveTypeInfos);
)

I tried to use -whole-program-visibility [1] flag in the second llvm-lto2 RUN command (which is expected to see no devirtualization), and the test failed..

[1]

--- a/llvm/test/ThinLTO/X86/devirt_prevailing.ll
+++ b/llvm/test/ThinLTO/X86/devirt_prevailing.ll
@@ -32,9 +32,7 @@
 ; RUN: llvm-lto2  run summary.o -save-temps -pass-remarks=. \
 ; RUN:   -thinlto-threads=1 \
 ; RUN:   -o tmp \
-; RUN:   --whole-program-visibility-enabled-in-lto=true \
-; RUN:   --validate-all-vtables-have-type-infos=true \
-; RUN:   --all-vtables-have-type-infos=true \
+; RUN:  -whole-program-visibility \
 ; RUN:   -r=summary.o,_ZN4Base8dispatchEv,px \
 ; RUN:   -r=summary.o,_ZN8DerivedN5printEv,px \
 ; RUN:   -r=summary.o,_ZTV8DerivedN,p \

if (ValidateAllVtablesHaveTypeInfos.getNumOccurrences() > 0)
Conf.ValidateAllVtablesHaveTypeInfos = ValidateAllVtablesHaveTypeInfos;
Copy link
Contributor

@teresajohnson teresajohnson Mar 20, 2025

Choose a reason for hiding this comment

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

I don't think either of these are needed for the test as ValidateAllVtablesHaveTypeInfos defaults to false, meaning we don't do this validation, and AllVtablesHaveTypeInfos is only checked if ValidateAllVtablesHaveTypeInfos is true.

if (AllVtablesHaveTypeInfos.getNumOccurrences() > 0)
Conf.AllVtablesHaveTypeInfos = AllVtablesHaveTypeInfos;

ThinBackend Backend;
if (ThinLTODistributedIndexes)
Backend = createWriteIndexesThinBackend(llvm::hardware_concurrency(Threads),
Expand Down
Loading