WholeProgramDevirt: Fix importing in llvm.type.checked.load case. #144019
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.6-beta.1
llvmbot
added
LTO
|
@llvm/pr-subscribers-llvm-transforms Author: Peter Collingbourne (pcc) ChangesWe were clearing SummaryTypeCheckedLoadUsers to prevent devirtualized Full diff: https://github.com/llvm/llvm-project/pull/144019.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index a7d9f3ba24b24..d01907df0c7de 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -494,28 +494,28 @@ struct CallSiteInfo {
/// Whether all call sites represented by this CallSiteInfo, including those
/// in summaries, have been devirtualized. This starts off as true because a
/// default constructed CallSiteInfo represents no call sites.
+ ///
+ /// If at the end of the pass there are still undevirtualized calls, we will
+ /// need to add a use of llvm.type.test to each of the function summaries in
+ /// the vector.
bool AllCallSitesDevirted = true;
// These fields are used during the export phase of ThinLTO and reflect
// information collected from function summaries.
- /// Whether any function summary contains an llvm.assume(llvm.type.test) for
- /// this slot.
- bool SummaryHasTypeTestAssumeUsers = false;
-
/// CFI-specific: a vector containing the list of function summaries that use
/// the llvm.type.checked.load intrinsic and therefore will require
/// resolutions for llvm.type.test in order to implement CFI checks if
- /// devirtualization was unsuccessful. If devirtualization was successful, the
- /// pass will clear this vector by calling markDevirt(). If at the end of the
- /// pass the vector is non-empty, we will need to add a use of llvm.type.test
- /// to each of the function summaries in the vector.
+ /// devirtualization was unsuccessful.
std::vector<FunctionSummary *> SummaryTypeCheckedLoadUsers;
+
+ /// A vector containing the list of function summaries that use
+ /// assume(llvm.type.test).
std::vector<FunctionSummary *> SummaryTypeTestAssumeUsers;
bool isExported() const {
- return SummaryHasTypeTestAssumeUsers ||
- !SummaryTypeCheckedLoadUsers.empty();
+ return !SummaryTypeCheckedLoadUsers.empty() ||
+ !SummaryTypeTestAssumeUsers.empty();
}
void addSummaryTypeCheckedLoadUser(FunctionSummary *FS) {
@@ -525,15 +525,11 @@ struct CallSiteInfo {
void addSummaryTypeTestAssumeUser(FunctionSummary *FS) {
SummaryTypeTestAssumeUsers.push_back(FS);
- SummaryHasTypeTestAssumeUsers = true;
AllCallSitesDevirted = false;
}
void markDevirt() {
AllCallSitesDevirted = true;
-
- // As explained in the comment for SummaryTypeCheckedLoadUsers.
- SummaryTypeCheckedLoadUsers.clear();
}
};
@@ -2465,11 +2461,14 @@ bool DevirtModule::run() {
if (ExportSummary && isa<MDString>(S.first.TypeID)) {
auto GUID = GlobalValue::getGUIDAssumingExternalLinkage(
cast<MDString>(S.first.TypeID)->getString());
- for (auto *FS : S.second.CSInfo.SummaryTypeCheckedLoadUsers)
- FS->addTypeTest(GUID);
+ auto AddTypeTestsForTypeCheckedLoads = [&](CallSiteInfo &CSI) {
+ if (!CSI.AllCallSitesDevirted)
+ for (auto *FS : CSI.SummaryTypeCheckedLoadUsers)
+ FS->addTypeTest(GUID);
+ };
+ AddTypeTestsForTypeCheckedLoads(S.second.CSInfo);
for (auto &CCS : S.second.ConstCSInfo)
- for (auto *FS : CCS.second.SummaryTypeCheckedLoadUsers)
- FS->addTypeTest(GUID);
+ AddTypeTestsForTypeCheckedLoads(CCS.second);
}
}
diff --git a/llvm/test/ThinLTO/X86/Inputs/devirt_single_hybrid_foo_tcl.ll b/llvm/test/ThinLTO/X86/Inputs/devirt_single_hybrid_foo_tcl.ll
new file mode 100644
index 0000000000000..4a696837bc8e9
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/Inputs/devirt_single_hybrid_foo_tcl.ll
@@ -0,0 +1,31 @@
+; ModuleID = 'foo.cpp'
+source_filename = "foo.cpp"
+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-unknown-linux-gnu"
+
+%struct.A = type { ptr }
+
+; Function Attrs: uwtable
+define hidden i32 @_Z3fooP1A(ptr %pA) local_unnamed_addr {
+entry:
+ %vtable = load ptr, ptr %pA, align 8, !tbaa !2
+ %0 = call { ptr, i1 } @llvm.type.checked.load(ptr %vtable, i32 0, metadata !"_ZTS1A")
+ %1 = extractvalue { ptr, i1 } %0, 0
+ %call = tail call i32 %1(ptr %pA)
+ %add = add nsw i32 %call, 10
+ ret i32 %add
+}
+
+declare { ptr, i1 } @llvm.type.checked.load(ptr, i32, metadata)
+
+; Function Attrs: nounwind willreturn
+declare void @llvm.assume(i1)
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 10.0.0 (trunk 373596)"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"vtable pointer", !4, i64 0}
+!4 = !{!"Simple C++ TBAA"}
diff --git a/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll b/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
index 90fdf0d7dfa09..fe3242e93159a 100644
--- a/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
+++ b/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
@@ -2,8 +2,28 @@
; when we're running hybrid LTO.
;
; RUN: opt -thinlto-bc -thinlto-split-lto-unit %s -o %t-main.bc
-; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo.ll -o %t-foo.bc
; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_bar.ll -o %t-bar.bc
+;
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo.ll -o %t-foo.bc
+; RUN: llvm-lto2 run -save-temps %t-main.bc %t-foo.bc %t-bar.bc -pass-remarks=. -o %t \
+; RUN: -whole-program-visibility \
+; RUN: -r=%t-foo.bc,_Z3fooP1A,pl \
+; RUN: -r=%t-main.bc,main,plx \
+; RUN: -r=%t-main.bc,_Z3barv,l \
+; RUN: -r=%t-bar.bc,_Z3barv,pl \
+; RUN: -r=%t-bar.bc,_Z3fooP1A, \
+; RUN: -r=%t-bar.bc,_ZNK1A1fEv,pl \
+; RUN: -r=%t-bar.bc,_ZTV1A,l \
+; RUN: -r=%t-bar.bc,_ZTVN10__cxxabiv117__class_type_infoE, \
+; RUN: -r=%t-bar.bc,_ZTS1A,pl \
+; RUN: -r=%t-bar.bc,_ZTI1A,pl \
+; RUN: -r=%t-bar.bc,_ZNK1A1fEv, \
+; RUN: -r=%t-bar.bc,_ZTV1A,pl \
+; RUN: -r=%t-bar.bc,_ZTI1A, 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: llvm-dis %t.1.3.import.bc -o - | FileCheck %s --check-prefix=IMPORT
+; RUN: llvm-dis %t.1.5.precodegen.bc -o - | FileCheck %s --check-prefix=CODEGEN
+
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo_tcl.ll -o %t-foo.bc
; RUN: llvm-lto2 run -save-temps %t-main.bc %t-foo.bc %t-bar.bc -pass-remarks=. -o %t \
; RUN: -whole-program-visibility \
; RUN: -r=%t-foo.bc,_Z3fooP1A,pl \
|
|
@llvm/pr-subscribers-lto Author: Peter Collingbourne (pcc) ChangesWe were clearing SummaryTypeCheckedLoadUsers to prevent devirtualized Full diff: https://github.com/llvm/llvm-project/pull/144019.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
index a7d9f3ba24b24..d01907df0c7de 100644
--- a/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
+++ b/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
@@ -494,28 +494,28 @@ struct CallSiteInfo {
/// Whether all call sites represented by this CallSiteInfo, including those
/// in summaries, have been devirtualized. This starts off as true because a
/// default constructed CallSiteInfo represents no call sites.
+ ///
+ /// If at the end of the pass there are still undevirtualized calls, we will
+ /// need to add a use of llvm.type.test to each of the function summaries in
+ /// the vector.
bool AllCallSitesDevirted = true;
// These fields are used during the export phase of ThinLTO and reflect
// information collected from function summaries.
- /// Whether any function summary contains an llvm.assume(llvm.type.test) for
- /// this slot.
- bool SummaryHasTypeTestAssumeUsers = false;
-
/// CFI-specific: a vector containing the list of function summaries that use
/// the llvm.type.checked.load intrinsic and therefore will require
/// resolutions for llvm.type.test in order to implement CFI checks if
- /// devirtualization was unsuccessful. If devirtualization was successful, the
- /// pass will clear this vector by calling markDevirt(). If at the end of the
- /// pass the vector is non-empty, we will need to add a use of llvm.type.test
- /// to each of the function summaries in the vector.
+ /// devirtualization was unsuccessful.
std::vector<FunctionSummary *> SummaryTypeCheckedLoadUsers;
+
+ /// A vector containing the list of function summaries that use
+ /// assume(llvm.type.test).
std::vector<FunctionSummary *> SummaryTypeTestAssumeUsers;
bool isExported() const {
- return SummaryHasTypeTestAssumeUsers ||
- !SummaryTypeCheckedLoadUsers.empty();
+ return !SummaryTypeCheckedLoadUsers.empty() ||
+ !SummaryTypeTestAssumeUsers.empty();
}
void addSummaryTypeCheckedLoadUser(FunctionSummary *FS) {
@@ -525,15 +525,11 @@ struct CallSiteInfo {
void addSummaryTypeTestAssumeUser(FunctionSummary *FS) {
SummaryTypeTestAssumeUsers.push_back(FS);
- SummaryHasTypeTestAssumeUsers = true;
AllCallSitesDevirted = false;
}
void markDevirt() {
AllCallSitesDevirted = true;
-
- // As explained in the comment for SummaryTypeCheckedLoadUsers.
- SummaryTypeCheckedLoadUsers.clear();
}
};
@@ -2465,11 +2461,14 @@ bool DevirtModule::run() {
if (ExportSummary && isa<MDString>(S.first.TypeID)) {
auto GUID = GlobalValue::getGUIDAssumingExternalLinkage(
cast<MDString>(S.first.TypeID)->getString());
- for (auto *FS : S.second.CSInfo.SummaryTypeCheckedLoadUsers)
- FS->addTypeTest(GUID);
+ auto AddTypeTestsForTypeCheckedLoads = [&](CallSiteInfo &CSI) {
+ if (!CSI.AllCallSitesDevirted)
+ for (auto *FS : CSI.SummaryTypeCheckedLoadUsers)
+ FS->addTypeTest(GUID);
+ };
+ AddTypeTestsForTypeCheckedLoads(S.second.CSInfo);
for (auto &CCS : S.second.ConstCSInfo)
- for (auto *FS : CCS.second.SummaryTypeCheckedLoadUsers)
- FS->addTypeTest(GUID);
+ AddTypeTestsForTypeCheckedLoads(CCS.second);
}
}
diff --git a/llvm/test/ThinLTO/X86/Inputs/devirt_single_hybrid_foo_tcl.ll b/llvm/test/ThinLTO/X86/Inputs/devirt_single_hybrid_foo_tcl.ll
new file mode 100644
index 0000000000000..4a696837bc8e9
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/Inputs/devirt_single_hybrid_foo_tcl.ll
@@ -0,0 +1,31 @@
+; ModuleID = 'foo.cpp'
+source_filename = "foo.cpp"
+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-unknown-linux-gnu"
+
+%struct.A = type { ptr }
+
+; Function Attrs: uwtable
+define hidden i32 @_Z3fooP1A(ptr %pA) local_unnamed_addr {
+entry:
+ %vtable = load ptr, ptr %pA, align 8, !tbaa !2
+ %0 = call { ptr, i1 } @llvm.type.checked.load(ptr %vtable, i32 0, metadata !"_ZTS1A")
+ %1 = extractvalue { ptr, i1 } %0, 0
+ %call = tail call i32 %1(ptr %pA)
+ %add = add nsw i32 %call, 10
+ ret i32 %add
+}
+
+declare { ptr, i1 } @llvm.type.checked.load(ptr, i32, metadata)
+
+; Function Attrs: nounwind willreturn
+declare void @llvm.assume(i1)
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{!"clang version 10.0.0 (trunk 373596)"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"vtable pointer", !4, i64 0}
+!4 = !{!"Simple C++ TBAA"}
diff --git a/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll b/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
index 90fdf0d7dfa09..fe3242e93159a 100644
--- a/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
+++ b/llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
@@ -2,8 +2,28 @@
; when we're running hybrid LTO.
;
; RUN: opt -thinlto-bc -thinlto-split-lto-unit %s -o %t-main.bc
-; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo.ll -o %t-foo.bc
; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_bar.ll -o %t-bar.bc
+;
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo.ll -o %t-foo.bc
+; RUN: llvm-lto2 run -save-temps %t-main.bc %t-foo.bc %t-bar.bc -pass-remarks=. -o %t \
+; RUN: -whole-program-visibility \
+; RUN: -r=%t-foo.bc,_Z3fooP1A,pl \
+; RUN: -r=%t-main.bc,main,plx \
+; RUN: -r=%t-main.bc,_Z3barv,l \
+; RUN: -r=%t-bar.bc,_Z3barv,pl \
+; RUN: -r=%t-bar.bc,_Z3fooP1A, \
+; RUN: -r=%t-bar.bc,_ZNK1A1fEv,pl \
+; RUN: -r=%t-bar.bc,_ZTV1A,l \
+; RUN: -r=%t-bar.bc,_ZTVN10__cxxabiv117__class_type_infoE, \
+; RUN: -r=%t-bar.bc,_ZTS1A,pl \
+; RUN: -r=%t-bar.bc,_ZTI1A,pl \
+; RUN: -r=%t-bar.bc,_ZNK1A1fEv, \
+; RUN: -r=%t-bar.bc,_ZTV1A,pl \
+; RUN: -r=%t-bar.bc,_ZTI1A, 2>&1 | FileCheck %s --check-prefix=REMARK
+; RUN: llvm-dis %t.1.3.import.bc -o - | FileCheck %s --check-prefix=IMPORT
+; RUN: llvm-dis %t.1.5.precodegen.bc -o - | FileCheck %s --check-prefix=CODEGEN
+
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo_tcl.ll -o %t-foo.bc
; RUN: llvm-lto2 run -save-temps %t-main.bc %t-foo.bc %t-bar.bc -pass-remarks=. -o %t \
; RUN: -whole-program-visibility \
; RUN: -r=%t-foo.bc,_Z3fooP1A,pl \
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
teresajohnson
approved these changes
Jun 13, 2025
| ; RUN: llvm-dis %t.1.3.import.bc -o - | FileCheck %s --check-prefix=IMPORT | ||
| ; RUN: llvm-dis %t.1.5.precodegen.bc -o - | FileCheck %s --check-prefix=CODEGEN | ||
|
|
||
| ; RUN: opt -thinlto-bc -thinlto-split-lto-unit %p/Inputs/devirt_single_hybrid_foo_tcl.ll -o %t-foo.bc |
There was a problem hiding this comment.
Please add a comment here about what this is testing vs the prior test.
Created using spr 1.3.6-beta.1
pcc
deleted the
users/pcc/spr/wholeprogramdevirt-fix-importing-in-llvmtypecheckedload-case
branch
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 13, 2025
…d case. We were clearing SummaryTypeCheckedLoadUsers to prevent devirtualized llvm.type.checked.load calls from being converted to llvm.type.test, which meant that AddCalls would not see them in the list of callsites and they would not get imported. Fix that by not clearing SummaryTypeCheckedLoadUsers so that the list survives to AddCalls and using AllCallSitesDevirted to control whether to convert them instead. Reviewers: teresajohnson Reviewed By: teresajohnson Pull Request: llvm/llvm-project#144019
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
We were clearing SummaryTypeCheckedLoadUsers to prevent devirtualized llvm.type.checked.load calls from being converted to llvm.type.test, which meant that AddCalls would not see them in the list of callsites and they would not get imported. Fix that by not clearing SummaryTypeCheckedLoadUsers so that the list survives to AddCalls and using AllCallSitesDevirted to control whether to convert them instead. Reviewers: teresajohnson Reviewed By: teresajohnson Pull Request: llvm#144019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were clearing SummaryTypeCheckedLoadUsers to prevent devirtualized
llvm.type.checked.load calls from being converted to llvm.type.test,
which meant that AddCalls would not see them in the list of
callsites and they would not get imported. Fix that by not clearing
SummaryTypeCheckedLoadUsers so that the list survives to AddCalls and
using AllCallSitesDevirted to control whether to convert them instead.