Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion llvm/lib/LTO/LTO.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1905,7 +1905,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.ExportDynamic);
Copy link
Contributor

Choose a reason for hiding this comment

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

The same fix should presumably go in for regular LTO (see the other def of IsVisibleToRegularObj in this file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, and added test coverage for hybrid LTO. Regular LTO test coverage is not very meaningful, since without this change, the VisibleOutsideSummary bit suppresses devirtualization with -Wl,--lto-whole-program-visibility -Wl,--lto-validate-all-vtables-have-type-infos in lld

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per offline discussion, opt -module-summary is the way to create LTO input for regular LTO. Now the regression test has coverage for {thin, hybrid, regular} LTO.

};

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

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

; RUN: llvm-dis -o - summary.o

;; TODO: Implement the fix for WPD in regular or hybrid LTO, and add test coverage.

; Index based WPD
; For `_ZTI7Derived`, the 'llvm-lto2' resolution arguments specifies `VisibleOutsideSummary` as false
; and `ExportDynamic` as false. The callsite inside @_ZN4Base8dispatchEv
; got devirtualized.
; RUN: llvm-lto2 run summary.o -save-temps -pass-remarks=. \
; 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,__cxa_pure_virtual, \
; RUN: -r=summary.o,_ZN8DerivedNC2Ev,x \
; RUN: -r=summary.o,_ZN4Base8dispatchEv,px \
; RUN: -r=summary.o,_ZN7DerivedC2Ev, \
; RUN: -r=summary.o,_ZN8DerivedN5printEv,px \
; RUN: -r=summary.o,_ZTS4Base, \
; RUN: -r=summary.o,_ZTV8DerivedN,p \
; RUN: -r=summary.o,_ZTI8DerivedN,p \
; RUN: -r=summary.o,_ZTI4Base, \
; RUN: -r=summary.o,_ZTS8DerivedN,p \
; RUN: -r=summary.o,_ZTI7Derived, \
; RUN: -r=summary.o,_ZTV4Base 2>&1 | FileCheck --allow-empty %s --check-prefix=REMARK

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

; Index based WPD
; For `_ZTI7Derived`, the 'llvm-lto2' resolution arguments specifies `VisibleOutsideSummary` as false
; and `ExportDynamic` as true. The callsite inside @_ZN4Base8dispatchEv won't
; get devirtualized.
; RUN: llvm-lto2 run summary.o -save-temps -pass-remarks=. \
; 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,__cxa_pure_virtual, \
; RUN: -r=summary.o,_ZN8DerivedNC2Ev,x \
; RUN: -r=summary.o,_ZN4Base8dispatchEv,px \
; RUN: -r=summary.o,_ZN7DerivedC2Ev, \
; RUN: -r=summary.o,_ZN8DerivedN5printEv,px \
; RUN: -r=summary.o,_ZTS4Base, \
; RUN: -r=summary.o,_ZTV8DerivedN,p \
; RUN: -r=summary.o,_ZTI8DerivedN,p \
; RUN: -r=summary.o,_ZTI4Base, \
; RUN: -r=summary.o,_ZTS8DerivedN,p \
; RUN: -r=summary.o,_ZTI7Derived,d \
; RUN: -r=summary.o,_ZTV4Base 2>&1 | FileCheck %s --allow-empty --implicit-check-not='single-impl: devirtualized a call to'

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, !type !4, !type !5, !vcall_visibility !6
@_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
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about this...presumably the GlobalResolution Prevailing flag and isPrevailingIRSymbol are false for _ZTI7Derived. Should we more broadly be returning true to IsVisibleToRegularObj for that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question about the prevailing bit. Let me think about it more and implement the change if needed as a separate patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, I'll look into whether checking Prevailing bit is a better alternative to ExportDynamic.

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 updated LTO.cpp to use IsPrevailing bit since it's semantically correct to do that.

@_ZTV4Base = linkonce_odr hidden constant { [3 x ptr] } { [3 x ptr] [ptr null, ptr @_ZTI4Base, ptr @__cxa_pure_virtual] }, !type !0, !type !1, !vcall_visibility !6
@_ZTI4Base = linkonce_odr hidden constant { ptr, ptr } { ptr null, ptr @_ZTS4Base }
@_ZTS4Base = linkonce_odr hidden constant [6 x i8] c"4Base\00", align 1

@llvm.used = appending global [1 x ptr] [ptr @_ZN8DerivedNC2Ev], 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 @_ZN8DerivedNC2Ev(ptr %this) #0 {
entry:
%this.addr = alloca ptr
store ptr %this, ptr %this.addr
%this1 = load ptr, ptr %this.addr
call void @_ZN7DerivedC2Ev(ptr %this1)
store ptr getelementptr inbounds inrange(-16, 8) ({ [3 x ptr] }, ptr @_ZTV8DerivedN, i32 0, i32 0, i32 2), ptr %this1
ret void
}

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

attributes #0 = { noinline optnone }

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

!0 = !{i64 16, !"_ZTS4Base"}
!1 = !{i64 16, !"_ZTSM4BaseFvvE.virtual"}
!2 = !{i64 16, !"_ZTS7Derived"}
!3 = !{i64 16, !"_ZTSM7DerivedFvvE.virtual"}
!4 = !{i64 16, !"_ZTS8DerivedN"}
!5 = !{i64 16, !"_ZTSM8DerivedNFvvE.virtual"}
!6 = !{i64 0}
21 changes: 21 additions & 0 deletions llvm/tools/llvm-lto2/llvm-lto2.cpp
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 @@ -257,6 +269,8 @@ static int run(int argc, char **argv) {
Res.VisibleToRegularObj = true;
else if (C == 'r')
Res.LinkerRedefined = true;
else if (C == 'd')
Res.ExportDynamic = true;
else {
llvm::errs() << "invalid character " << C << " in resolution: " << R
<< '\n';
Expand Down Expand Up @@ -332,6 +346,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

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

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 (

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

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