[SPARC] Properly handle CC for long double on sparc32 #162226
Merged
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
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-llvm-adt @llvm/pr-subscribers-backend-sparc Author: Koakuma (koachan) ChangesPass and return This should fix the issue at #41838. Full diff: https://github.com/llvm/llvm-project/pull/162226.diff 6 Files Affected:
diff --git a/clang/lib/Basic/Targets/Sparc.h b/clang/lib/Basic/Targets/Sparc.h
index 3215e648ba6c3..acc27194c38ea 100644
--- a/clang/lib/Basic/Targets/Sparc.h
+++ b/clang/lib/Basic/Targets/Sparc.h
@@ -166,6 +166,13 @@ class LLVM_LIBRARY_VISIBILITY SparcV8TargetInfo : public SparcTargetInfo {
PtrDiffType = SignedLong;
break;
}
+
+ // The SPARCv8 System V ABI has long double 128-bits in size, but 64-bit
+ // aligned.
+ LongDoubleWidth = 128;
+ LongDoubleAlign = 64;
+ LongDoubleFormat = &llvm::APFloat::IEEEquad();
+
// Up to 32 bits (V8) or 64 bits (V9) are lock-free atomic, but we're
// willing to do atomic ops on up to 64 bits.
MaxAtomicPromoteWidth = 64;
diff --git a/clang/lib/CodeGen/Targets/Sparc.cpp b/clang/lib/CodeGen/Targets/Sparc.cpp
index 38dbebdec2429..4259c8bbfdcae 100644
--- a/clang/lib/CodeGen/Targets/Sparc.cpp
+++ b/clang/lib/CodeGen/Targets/Sparc.cpp
@@ -26,6 +26,7 @@ class SparcV8ABIInfo : public DefaultABIInfo {
private:
ABIArgInfo classifyReturnType(QualType RetTy) const;
+ ABIArgInfo classifyArgumentType(QualType Ty) const;
void computeInfo(CGFunctionInfo &FI) const override;
};
} // end anonymous namespace
@@ -33,12 +34,33 @@ class SparcV8ABIInfo : public DefaultABIInfo {
ABIArgInfo
SparcV8ABIInfo::classifyReturnType(QualType Ty) const {
+ if (Ty->isRealFloatingType() && getContext().getTypeSize(Ty) == 128)
+ return getNaturalAlignIndirect(Ty, getDataLayout().getAllocaAddrSpace());
+
if (Ty->isAnyComplexType()) {
- return ABIArgInfo::getDirect();
- }
- else {
- return DefaultABIInfo::classifyReturnType(Ty);
+ auto AI = ABIArgInfo::getDirect();
+ AI.setInReg(true);
+ return AI;
}
+
+ return DefaultABIInfo::classifyReturnType(Ty);
+}
+
+ABIArgInfo SparcV8ABIInfo::classifyArgumentType(QualType Ty) const {
+ const BuiltinType *BT = Ty->getAs<BuiltinType>();
+ bool IsF128 = false;
+
+ if (Ty->isRealFloatingType() && getContext().getTypeSize(Ty) == 128)
+ IsF128 = true;
+
+ // FIXME not sure if redundant
+ if (BT && BT->getKind() == BuiltinType::LongDouble)
+ IsF128 = true;
+
+ if (IsF128)
+ return getNaturalAlignIndirect(Ty, getDataLayout().getAllocaAddrSpace());
+
+ return DefaultABIInfo::classifyArgumentType(Ty);
}
void SparcV8ABIInfo::computeInfo(CGFunctionInfo &FI) const {
diff --git a/compiler-rt/lib/builtins/CMakeLists.txt b/compiler-rt/lib/builtins/CMakeLists.txt
index 6c226aa7d2d48..790bf5758f4a2 100644
--- a/compiler-rt/lib/builtins/CMakeLists.txt
+++ b/compiler-rt/lib/builtins/CMakeLists.txt
@@ -960,9 +960,9 @@ else ()
list(APPEND BUILTIN_CFLAGS_${arch} -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET)
endif()
- # For RISCV32, we must force enable int128 for compiling long
+ # For RISCV32 and 32-bit SPARC, we must force enable int128 for compiling long
# double routines.
- if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" STREQUAL "riscv32")
+ if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" MATCHES "riscv32|sparc$" AND NOT CMAKE_COMPILER_IS_GNUCC)
list(APPEND BUILTIN_CFLAGS_${arch} -fforce-enable-int128)
endif()
diff --git a/compiler-rt/test/builtins/CMakeLists.txt b/compiler-rt/test/builtins/CMakeLists.txt
index 63f4c94605c90..5643bfc51ea81 100644
--- a/compiler-rt/test/builtins/CMakeLists.txt
+++ b/compiler-rt/test/builtins/CMakeLists.txt
@@ -44,7 +44,7 @@ foreach(arch ${BUILTIN_TEST_ARCH})
string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")
endif()
- if (COMPILER_RT_ENABLE_SOFTWARE_INT128 OR ${arch} STREQUAL "riscv32")
+ if (COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" MATCHES "riscv32|sparc$" AND NOT CMAKE_COMPILER_IS_GNUCC)
list(APPEND BUILTINS_TEST_TARGET_CFLAGS -fforce-enable-int128)
string(REPLACE ";" " " BUILTINS_TEST_TARGET_CFLAGS "${BUILTINS_TEST_TARGET_CFLAGS}")
endif()
diff --git a/llvm/lib/Target/Sparc/SparcCallingConv.td b/llvm/lib/Target/Sparc/SparcCallingConv.td
index 8afd0a7fc09ad..55be696c14a78 100644
--- a/llvm/lib/Target/Sparc/SparcCallingConv.td
+++ b/llvm/lib/Target/Sparc/SparcCallingConv.td
@@ -24,8 +24,8 @@ def CC_Sparc32 : CallingConv<[
// As are v2i32 arguments (this would be the default behavior for
// v2i32 if it wasn't allocated to the IntPair register-class)
CCIfType<[v2i32], CCCustom<"CC_Sparc_Assign_Split_64">>,
-
-
+ // f128 arguments are passed indirectly.
+ CCIfType<[f128], CCPassIndirect<i32>>,
// Alternatively, they are assigned to the stack in 4-byte aligned units.
CCAssignToStack<4, 4>
]>;
diff --git a/llvm/lib/Target/Sparc/SparcISelLowering.cpp b/llvm/lib/Target/Sparc/SparcISelLowering.cpp
index a1607097af1ef..6ce636f470896 100644
--- a/llvm/lib/Target/Sparc/SparcISelLowering.cpp
+++ b/llvm/lib/Target/Sparc/SparcISelLowering.cpp
@@ -554,20 +554,19 @@ SDValue SparcTargetLowering::LowerFormalArguments_32(
continue;
}
- int FI = MF.getFrameInfo().CreateFixedObject(4,
- Offset,
- true);
- SDValue FIPtr = DAG.getFrameIndex(FI, PtrVT);
- SDValue Load ;
+ int FI;
if (VA.getValVT() == MVT::i32 || VA.getValVT() == MVT::f32) {
- Load = DAG.getLoad(VA.getValVT(), dl, Chain, FIPtr, MachinePointerInfo());
+ FI = MF.getFrameInfo().CreateFixedObject(4, Offset, true);
} else if (VA.getValVT() == MVT::f128) {
- report_fatal_error("SPARCv8 does not handle f128 in calls; "
- "pass indirectly");
+ FI = MF.getFrameInfo().CreateFixedObject(16, Offset, false);
} else {
// We shouldn't see any other value types here.
llvm_unreachable("Unexpected ValVT encountered in frame lowering.");
}
+
+ SDValue FIPtr = DAG.getFrameIndex(FI, PtrVT);
+ SDValue Load =
+ DAG.getLoad(VA.getValVT(), dl, Chain, FIPtr, MachinePointerInfo());
InVals.push_back(Load);
}
@@ -913,7 +912,9 @@ SparcTargetLowering::LowerCall_32(TargetLowering::CallLoweringInfo &CLI,
// Promote the value if needed.
switch (VA.getLocInfo()) {
default: llvm_unreachable("Unknown loc info!");
- case CCValAssign::Full: break;
+ case CCValAssign::Full:
+ case CCValAssign::Indirect:
+ break;
case CCValAssign::SExt:
Arg = DAG.getNode(ISD::SIGN_EXTEND, dl, VA.getLocVT(), Arg);
break;
|
Contributor
Author
|
Tagging this as WIP since there's some parts that's incomplete, I just want to solicit some advice for now. The status is that for C code (e.g I'm still seeing miscompilations: |
koachan
commented
Oct 7, 2025
Contributor
Author
I think the main issue here is that the fp128s need to be passed around as if it's a sret struct but I dunno how to declare it in (Ret)CC_Sparc32... |
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
If SparcTargetLowering::CanLowerReturn fails, the value should be automatically converted to sret without any explicit code.
| // The SPARCv8 System V ABI has long double 128-bits in size, but 64-bit | ||
| // aligned. | ||
| LongDoubleWidth = 128; | ||
| LongDoubleAlign = 64; |
Collaborator
There was a problem hiding this comment.
You probably also need to fix the datalayout
Contributor
Author
There was a problem hiding this comment.
It already has the f128:64 part, so I think I'm good here?
| } else if (VA.getValVT() == MVT::f128) { | ||
| report_fatal_error("SPARCv8 does not handle f128 in calls; " | ||
| "pass indirectly"); | ||
| FI = MF.getFrameInfo().CreateFixedObject(16, Offset, false); |
Collaborator
There was a problem hiding this comment.
This doesn't look right.
If the value is passed indirectly, that's encoded in VA: call getLocInfo(), check for CCValAssign::Indirect. If it's indirect, the argument is a pointer, so you need to load the pointer, then load the underlying value from that pointer.
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Looks okay at first glance. Take the [WIP] out of the title when you're ready for a full review.
clang/lib/CodeGen/Targets/Sparc.cpp
Outdated
|
|
||
| // FIXME not sure if redundant | ||
| if (BT && BT->getKind() == BuiltinType::LongDouble) | ||
| IsF128 = true; |
Collaborator
There was a problem hiding this comment.
Yes, this looks redundant. isRealFloatingType() checks basically the same thing.
koachan
changed the title
Contributor
Author
|
Okay, I think this should be good enough for a full review, taking off the WIP tag.
Whoops, my bad. I meant to do it yesterday but I forgot~ |
Collaborator
|
Thanks a lot for finally fixing this. I've now run a full regtest on sparcv9-sun-solaris2.11, with generally excellent results: The The new The code in question is I haven't looked closer yet. |
Contributor
Author
I think this is the @efriedma-quic is there a way for us to disambiguate it in |
llvmbot
added
compiler-rt:ubsan
Collaborator
|
Maybe you want CCIfConsecutiveRegs? |
Contributor
Author
Wait, so how do I do it? |
Collaborator
|
I think it's something like that? Not confident; haven't touched this in a while. |
Contributor
Author
|
Okay, I think the |
koachan
commented
Oct 29, 2025
| ; SPARC32-NEXT: add %fp, -32, %i1 | ||
| ; SPARC32-NEXT: st %i1, [%sp+92] | ||
| ; SPARC32-NEXT: std %f4, [%fp+-24] | ||
| ; SPARC32-NEXT: call sinl |
Contributor
Author
There was a problem hiding this comment.
So sinl here (along with cosl and friends) is a C function, so the call should follow C ABI rules (e.g. it needs to have a trailing unimp), but here as you can see it violates it.
Curiously, manually written calls in C seems to be lowered just fine by LLVM, the wrong lowering only happens when LLVM inserts its own calls automatically. What should I do with it?
Contributor
There was a problem hiding this comment.
IIUC the generated call returns the value in registers, which is already wrong?
Contributor
Author
There was a problem hiding this comment.
Yeah, that too.
sinl here is a C function but LLVM seems to be calling it using its own calling convention, instead of following the C one.
s-barannikov
reviewed
Nov 9, 2025
| ; SPARC32-NEXT: add %fp, -32, %i1 | ||
| ; SPARC32-NEXT: st %i1, [%sp+92] | ||
| ; SPARC32-NEXT: std %f4, [%fp+-24] | ||
| ; SPARC32-NEXT: call sinl |
Contributor
There was a problem hiding this comment.
IIUC the generated call returns the value in registers, which is already wrong?
Contributor
You need to return false for it in |
s-barannikov
approved these changes
Nov 26, 2025
Contributor
s-barannikov
left a comment
s-barannikov
left a comment
There was a problem hiding this comment.
LGTM with one question
| # For RISCV32 and 32-bit SPARC, we must force enable int128 for compiling long | ||
| # double routines. | ||
| if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" STREQUAL "riscv32") | ||
| if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" STREQUAL "riscv32" OR ("${arch}" STREQUAL "sparc" AND NOT CMAKE_COMPILER_IS_GNUCC)) |
Contributor
There was a problem hiding this comment.
Sorry for going back and forth, I suppose -fforce-enable-in128 isn't supported by riscv-gcc either, so it should be the same as in the file below.
When compiled with gcc, how is the library supposed to link if int128 routines are not compiled in?
Contributor
Author
There was a problem hiding this comment.
When compiled with gcc, how is the library supposed to link if int128 routines are not compiled in?
I suppose in that case the library will lack long double routines? @rorth probably know better about this.
Collaborator
There was a problem hiding this comment.
When compiled with gcc, how is the library supposed to link if int128 routines are not compiled in?
I suppose in that case the library will lack
long doubleroutines? @rorth probably know better about this.
I've no idea right now (have tried to forget all of this ;-). Besides, I'm so busy with the upcoming GCC 16 release that I've no time left for LLVM.
What I've done in the past is do build with both clang and gcc as build compiler, 2-stage builds with clang, 1-stage ones with gcc. And note that the compiler-rt tests still aren't run in runtime builds (which is the default these days), not even a target to do so manually, so I've always used
-DLLVM_ENABLE_PROJECTS=compiler-rt
to avoid that.
Contributor
Author
There was a problem hiding this comment.
Thanks for the hint! I tried running it and seems like I found some other long double related failures:
SanitizerCommon-asan-sparc-Linux :: printf-ldbl.c
SanitizerCommon-asan-sparc-Linux :: scanf-ldbl.c
SanitizerCommon-ubsan-sparc-Linux :: printf-ldbl.c
SanitizerCommon-ubsan-sparc-Linux :: scanf-ldbl.c
Because clang lowers the printf/scanf calls into something like __nldbl_snprintf and __nldbl___isoc99_sscanf...
Seems like I missed defining __LONG_DOUBLE_128__, lemme fix this.
Contributor
Author
There was a problem hiding this comment.
Okay seems like after __LONG_DOUBLE_128__ is added the issue is gone.
Contributor
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/19621 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/10834 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/14/builds/4772 Here is the relevant piece of the build log for the reference |
koachan
commented
Nov 30, 2025
| # For RISCV32 and 32-bit SPARC, we must force enable int128 for compiling long | ||
| # double routines. | ||
| if(COMPILER_RT_ENABLE_SOFTWARE_INT128 OR "${arch}" STREQUAL "riscv32") | ||
| if (COMPILER_RT_ENABLE_SOFTWARE_INT128 OR ("${arch}" MATCHES "riscv32|sparc$" |
Contributor
Author
There was a problem hiding this comment.
From this comment:
LLVM Buildbot has detected a new failure on builder
clang-solaris11-sparcv9running onsolaris11-sparcv9while buildingclang,compiler-rt,llvmat step 5 "ninja check 1".Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/10834
Here is the relevant piece of the build log for the reference
The errors seem to be about missing symbols for 128-bit ops like __fixunstfdi, __multc3, and friends.
Solaris seems to use sparcv9 for the arch, so I guess I'll add it too.
aahrun
pushed a commit
to aahrun/llvm-project
that referenced
this pull request
Dec 1, 2025
Pass and return `long double`s indirectly, as specified in the psABI. This continues the patch at https://reviews.llvm.org/D89130. This should fix the issue at llvm#41838.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
Pass and return
long doubles indirectly, as specified in the psABI.This continues the patch at https://reviews.llvm.org/D89130.
This should fix the issue at #41838.