Skip to content

[X86] Add ABI handling for __float128 to match with GCC #75156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 5, 2024
Merged

[X86] Add ABI handling for __float128 to match with GCC #75156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9860e54
[X86] Add ABI handling for fp128
phoebewang Dec 12, 2023
2619d68
Add non-SSE RUN
phoebewang Dec 21, 2023
30db4d0
Added release note
phoebewang Jan 5, 2024
af71a05
Fix typo
phoebewang Jan 5, 2024
488d1c3
Update release note
phoebewang Jan 5, 2024
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
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,7 @@ X86 Support
* Support intrinsic of ``_uwrmsr``.
- Support ISA of ``AVX10.1``.
- ``-march=pantherlake`` and ``-march=clearwaterforest`` are now supported.
- Added ABI handling for ``__float128`` to match with GCC.

Arm and AArch64 Support
^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/Targets/X86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1797,6 +1797,9 @@ void X86_64ABIInfo::classify(QualType Ty, uint64_t OffsetBase, Class &Lo,
} else if (k == BuiltinType::Float || k == BuiltinType::Double ||
k == BuiltinType::Float16 || k == BuiltinType::BFloat16) {
Current = SSE;
} else if (k == BuiltinType::Float128) {
Lo = SSE;
Hi = SSEUp;
} else if (k == BuiltinType::LongDouble) {
const llvm::fltSemantics *LDF = &getTarget().getLongDoubleFormat();
if (LDF == &llvm::APFloat::IEEEquad()) {
Expand Down
36 changes: 36 additions & 0 deletions clang/test/CodeGen/X86/fp128-abi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -target-feature +sse2 < %s | FileCheck %s --check-prefixes=CHECK
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth adding a non-SSE RUN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This patch only changes for 64-bit ABI, non-SSE is not a valid case for 64-bit.
OTOH, -sse just generate identical output with +sse2, which doesn't match with GCC https://godbolt.org/z/4nxnhnovd
I think we may need to add a semacheck for it. Do you think we should add it with this patch or a follow up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're dealing with it soon then handling it in a followup sounds good to me

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 decided not to report error for -sse after investigating the current diagnosis machinism.
We didn't report the SSE error in semacheck but in backend due to backend crash.
__float128 doesn't have crash issue and can be passed on GPR registers without SSE enabled.
IIUC, we prefer to be compatible with early version rather than always to GCC. So I'd like to keep the convention as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK - so we have test coverage for non-SSE cases already in the backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, e.g, llvm/test/CodeGen/X86/{soft-fp,x87}.ll, though I doubt if they can work in reality since they are calling to the same lib functions.

// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -target-feature -sse2 < %s | FileCheck %s --check-prefixes=CHECK

struct st1 {
__float128 a;
};

struct st1 h1(__float128 a) {
// CHECK: define{{.*}}fp128 @h1(fp128
struct st1 x;
x.a = a;
return x;
}

__float128 h2(struct st1 x) {
// CHECK: define{{.*}}fp128 @h2(fp128
return x.a;
}

struct st2 {
__float128 a;
int b;
};

struct st2 h3(__float128 a, int b) {
// CHECK: define{{.*}}void @h3(ptr {{.*}}sret(%struct.st2)
struct st2 x;
x.a = a;
x.b = b;
return x;
}

__float128 h4(struct st2 x) {
// CHECK: define{{.*}}fp128 @h4(ptr {{.*}}byval(%struct.st2)
return x.a;
}