[llvm] TargetLibraryInfo: Assume no libcalls in the default constructor (PR #92400)
Matt Arsenault via llvm-commits
llvm-commits at lists.llvm.org
Thu May 16 08:13:28 PDT 2024
https://github.com/arsenm updated https://github.com/llvm/llvm-project/pull/92400
>From 57a58e057d3af1b4208a74c89af26176c959fac8 Mon Sep 17 00:00:00 2001
From: Matt Arsenault <Matthew.Arsenault at amd.com>
Date: Thu, 16 May 2024 17:04:24 +0200
Subject: [PATCH 1/3] PlaceSafepoints: Add baseline test that shows
TargetLibraryInfo is wrong
---
llvm/test/Transforms/PlaceSafepoints/libcall.ll | 3 +++
1 file changed, 3 insertions(+)
diff --git a/llvm/test/Transforms/PlaceSafepoints/libcall.ll b/llvm/test/Transforms/PlaceSafepoints/libcall.ll
index 89090ba1b243c..fa1ddde62121e 100644
--- a/llvm/test/Transforms/PlaceSafepoints/libcall.ll
+++ b/llvm/test/Transforms/PlaceSafepoints/libcall.ll
@@ -1,5 +1,8 @@
; RUN: opt -S -passes=place-safepoints < %s | FileCheck %s
+; FIXME: This should fail without the ldexp builtin
+; RUN: opt -S -passes=place-safepoints -disable-builtin=ldexp < %s | FileCheck %s
+
; Libcalls will not contain a safepoint poll, so check that we insert
; a safepoint in a loop containing a libcall.
declare double @ldexp(double %x, i32 %n) nounwind readnone
>From 1e2ec61e82cb0946a953edf8bed40f4b552852b2 Mon Sep 17 00:00:00 2001
From: Matt Arsenault <Matthew.Arsenault at amd.com>
Date: Thu, 16 May 2024 17:05:15 +0200
Subject: [PATCH 2/3] PlaceSafepoints: Fix using default constructed
TargetLibraryInfo
PlaceSafepoints has an awful hack where it's creating a legacy
PassManager inside it's runImpl, which was not propagating the incoming
TLI. This means there's an implicit bug fix, where PlaceSafepoints would have
been treating too many calls as builtins.
---
llvm/include/llvm/Analysis/TargetLibraryInfo.h | 4 ++++
llvm/lib/Analysis/TargetLibraryInfo.cpp | 4 ++++
llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp | 2 ++
llvm/test/Transforms/PlaceSafepoints/libcall.ll | 8 ++++----
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/llvm/include/llvm/Analysis/TargetLibraryInfo.h b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
index 46f31f918e7b6..f5da222d11f55 100644
--- a/llvm/include/llvm/Analysis/TargetLibraryInfo.h
+++ b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
@@ -633,6 +633,10 @@ class TargetLibraryInfoWrapperPass : public ImmutablePass {
explicit TargetLibraryInfoWrapperPass(const Triple &T);
explicit TargetLibraryInfoWrapperPass(const TargetLibraryInfoImpl &TLI);
+ // FIXME: This should be removed when PlaceSafepoints is fixed to not create a
+ // PassManager inside a pass.
+ explicit TargetLibraryInfoWrapperPass(const TargetLibraryInfo &TLI);
+
TargetLibraryInfo &getTLI(const Function &F) {
FunctionAnalysisManager DummyFAM;
TLI = TLA.run(F, DummyFAM);
diff --git a/llvm/lib/Analysis/TargetLibraryInfo.cpp b/llvm/lib/Analysis/TargetLibraryInfo.cpp
index c62d9daa13ef0..684b0759f3157 100644
--- a/llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ b/llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -1383,6 +1383,10 @@ TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass(
initializeTargetLibraryInfoWrapperPassPass(*PassRegistry::getPassRegistry());
}
+TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass(
+ const TargetLibraryInfo &TLIOther)
+ : TargetLibraryInfoWrapperPass(*TLIOther.Impl) {}
+
AnalysisKey TargetLibraryAnalysis::Key;
// Register the basic pass.
diff --git a/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp b/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
index 77d155d7e78e3..dcdea6e7b62ae 100644
--- a/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
+++ b/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
@@ -288,6 +288,8 @@ bool PlaceSafepointsPass::runImpl(Function &F, const TargetLibraryInfo &TLI) {
// with for the moment.
legacy::FunctionPassManager FPM(F.getParent());
bool CanAssumeCallSafepoints = enableCallSafepoints(F);
+
+ FPM.add(new TargetLibraryInfoWrapperPass(TLI));
auto *PBS = new PlaceBackedgeSafepointsLegacyPass(CanAssumeCallSafepoints);
FPM.add(PBS);
FPM.run(F);
diff --git a/llvm/test/Transforms/PlaceSafepoints/libcall.ll b/llvm/test/Transforms/PlaceSafepoints/libcall.ll
index fa1ddde62121e..6e26f924a5ba3 100644
--- a/llvm/test/Transforms/PlaceSafepoints/libcall.ll
+++ b/llvm/test/Transforms/PlaceSafepoints/libcall.ll
@@ -1,8 +1,7 @@
-; RUN: opt -S -passes=place-safepoints < %s | FileCheck %s
-
-; FIXME: This should fail without the ldexp builtin
+; RUN: opt -S -passes=place-safepoints < %s | FileCheck -check-prefixes=CHECK,WITHLDEXPF %s
; RUN: opt -S -passes=place-safepoints -disable-builtin=ldexp < %s | FileCheck %s
+
; Libcalls will not contain a safepoint poll, so check that we insert
; a safepoint in a loop containing a libcall.
declare double @ldexp(double %x, i32 %n) nounwind readnone
@@ -20,7 +19,8 @@ loop:
; CHECK-NEXT: %x_loop = phi double [ %x, %entry ], [ %x_exp, %loop ]
; CHECK-NEXT: %x_exp = call double @ldexp(double %x_loop, i32 5)
; CHECK-NEXT: %done = fcmp ogt double %x_exp, 1.5
-; CHECK-NEXT: call void @do_safepoint
+; WITHLDEXPF-NEXT: call void @do_safepoint
+; CHECK-NEXT: br
%x_loop = phi double [ %x, %entry ], [ %x_exp, %loop ]
%x_exp = call double @ldexp(double %x_loop, i32 5) nounwind readnone
%done = fcmp ogt double %x_exp, 1.5
>From ae7bf324db72aa645fcf4596eed4f64e6f3bab28 Mon Sep 17 00:00:00 2001
From: Matt Arsenault <Matthew.Arsenault at amd.com>
Date: Thu, 16 May 2024 09:56:49 +0200
Subject: [PATCH 3/3] TargetLibraryInfo: Assume no libcalls in the default
constructor
The only tricky point here is PlaceSafepoints has an awful hack
where it's creating a legacy PassManager inside it's runImpl,
which was not propagating the incoming TLI. This means there's
an implicit bug fix, where PlaceSafepoints would have been
treating too many calls as builtins.
I'm trying to delete the default constructor altogether,
but this seems to be more difficult.
---
llvm/lib/Analysis/TargetLibraryInfo.cpp | 51 +++++++++++++++----------
1 file changed, 31 insertions(+), 20 deletions(-)
diff --git a/llvm/lib/Analysis/TargetLibraryInfo.cpp b/llvm/lib/Analysis/TargetLibraryInfo.cpp
index 684b0759f3157..7ce42447b6308 100644
--- a/llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ b/llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -160,11 +160,28 @@ bool TargetLibraryInfoImpl::isCallingConvCCompatible(Function *F) {
F->getFunctionType());
}
+static void initializeBase(TargetLibraryInfoImpl &TLI, const Triple &T) {
+ bool ShouldExtI32Param, ShouldExtI32Return;
+ bool ShouldSignExtI32Param, ShouldSignExtI32Return;
+ TargetLibraryInfo::initExtensionsForTriple(
+ ShouldExtI32Param, ShouldExtI32Return, ShouldSignExtI32Param,
+ ShouldSignExtI32Return, T);
+ TLI.setShouldExtI32Param(ShouldExtI32Param);
+ TLI.setShouldExtI32Return(ShouldExtI32Return);
+ TLI.setShouldSignExtI32Param(ShouldSignExtI32Param);
+ TLI.setShouldSignExtI32Return(ShouldSignExtI32Return);
+
+ // Let's assume by default that the size of int is 32 bits, unless the target
+ // is a 16-bit architecture because then it most likely is 16 bits. If that
+ // isn't true for a target those defaults should be overridden below.
+ TLI.setIntSize(T.isArch16Bit() ? 16 : 32);
+}
+
/// Initialize the set of available library functions based on the specified
/// target triple. This should be carefully written so that a missing target
/// triple gets a sane set of defaults.
-static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
- ArrayRef<StringLiteral> StandardNames) {
+static void initializeLibCalls(TargetLibraryInfoImpl &TLI, const Triple &T,
+ ArrayRef<StringLiteral> StandardNames) {
// Set IO unlocked variants as unavailable
// Set them as available per system below
TLI.setUnavailable(LibFunc_getc_unlocked);
@@ -178,20 +195,6 @@ static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
TLI.setUnavailable(LibFunc_fputs_unlocked);
TLI.setUnavailable(LibFunc_fgets_unlocked);
- bool ShouldExtI32Param, ShouldExtI32Return;
- bool ShouldSignExtI32Param, ShouldSignExtI32Return;
- TargetLibraryInfo::initExtensionsForTriple(ShouldExtI32Param,
- ShouldExtI32Return, ShouldSignExtI32Param, ShouldSignExtI32Return, T);
- TLI.setShouldExtI32Param(ShouldExtI32Param);
- TLI.setShouldExtI32Return(ShouldExtI32Return);
- TLI.setShouldSignExtI32Param(ShouldSignExtI32Param);
- TLI.setShouldSignExtI32Return(ShouldSignExtI32Return);
-
- // Let's assume by default that the size of int is 32 bits, unless the target
- // is a 16-bit architecture because then it most likely is 16 bits. If that
- // isn't true for a target those defaults should be overridden below.
- TLI.setIntSize(T.isArch16Bit() ? 16 : 32);
-
// There is really no runtime library on AMDGPU, apart from
// __kmpc_alloc/free_shared.
if (T.isAMDGPU()) {
@@ -882,11 +885,19 @@ static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
TLI.addVectorizableFunctionsFromVecLib(ClVectorLibrary, T);
}
-TargetLibraryInfoImpl::TargetLibraryInfoImpl() {
- // Default to everything being available.
- memset(AvailableArray, -1, sizeof(AvailableArray));
+/// Initialize the set of available library functions based on the specified
+/// target triple. This should be carefully written so that a missing target
+/// triple gets a sane set of defaults.
+static void initialize(TargetLibraryInfoImpl &TLI, const Triple &T,
+ ArrayRef<StringLiteral> StandardNames) {
+ initializeBase(TLI, T);
+ initializeLibCalls(TLI, T, StandardNames);
+}
- initialize(*this, Triple(), StandardNames);
+TargetLibraryInfoImpl::TargetLibraryInfoImpl() {
+ // Default to nothing being available.
+ memset(AvailableArray, 0, sizeof(AvailableArray));
+ initializeBase(*this, Triple());
}
TargetLibraryInfoImpl::TargetLibraryInfoImpl(const Triple &T) {
More information about the llvm-commits
mailing list