TargetLibraryInfo: Assume no libcalls in the default constructor #92400
Conversation
This file contains 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
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Matt Arsenault (arsenm) ChangesThe 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. Full diff: https://github.com/llvm/llvm-project/pull/92400.diff 3 Files Affected:
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..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) {
@@ -1383,6 +1394,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);
|
jhuber6
reviewed
May 16, 2024
Comment on lines
-886
to
-887
| // Default to everything being available. | ||
| memset(AvailableArray, -1, sizeof(AvailableArray)); |
There was a problem hiding this comment.
So we no longer default to everything being available? I'm not disagreeing with it because I think we need to select these more intelligently, but I'm wondering if that has an fallout.
There was a problem hiding this comment.
No, that's the next step I'm working towards
There was a problem hiding this comment.
No, that's the next step I'm working towards
Not sure I follow - what is the next step? afaict this PR does what @jhuber6 was describing (no longer default to everything being available).
There was a problem hiding this comment.
The default constructed TLI should never be used. It shouldn't be possible to construct one without a triple, but there are places where one is default constructed and never used.
The ideal follow up change would be to not make anything available with an unknown target in the real constructor
jhuber6
approved these changes
May 16, 2024
Comment on lines
198
to
200
| // There is really no runtime library on AMDGPU, apart from | ||
| // __kmpc_alloc/free_shared. | ||
| if (T.isAMDGPU()) { |
There was a problem hiding this comment.
Unrelated, but I'm wondering if / when we can turn some of these on given the GPU libc stuff.
Comment on lines
-886
to
-887
| // Default to everything being available. | ||
| memset(AvailableArray, -1, sizeof(AvailableArray)); |
There was a problem hiding this comment.
No, that's the next step I'm working towards
Not sure I follow - what is the next step? afaict this PR does what @jhuber6 was describing (no longer default to everything being available).
arsenm
force-pushed
the
tli-no-libcalls-in-default-constructor
branch
from
13328c7 to
ae7bf32
Compare
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.
arsenm
force-pushed
the
tli-no-libcalls-in-default-constructor
branch
from
ae7bf32 to
0114b68
Compare
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.
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.