Skip to content
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.

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

[CIR] [Lowering] [X86_64] VaArg for long double in x86 #1087

Closed
wants to merge 2 commits into from

Conversation

ChuanqiXu9
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 commented Nov 8, 2024

This is the following of #1086. Since I can't create branches to this repo so that I can't start real stacked PRs. After #1086 got merged, the PR will show the real change. This PR is for preview only.

After #1086, when we want to use LongDouble for VAArg, we will be in trouble due to details in X86_64's ABI and this patch tries to address this.

The practical impact the patch is, after this patch, with #1088 and a small following up fix, we can build and run all C's benchmark in SpecCPU 2017. I think it is a milestone.

@ChuanqiXu9 ChuanqiXu9 changed the title Va arg for long double in x86 [CIR] [Lowering] [X86_64] VaArg for long double in x86 Nov 8, 2024
@ChuanqiXu9 ChuanqiXu9 marked this pull request as draft November 8, 2024 03:00
@ChuanqiXu9 ChuanqiXu9 force-pushed the VAArgForLongDoubleInX86 branch from 2b69de9 to 7d28be4 Compare November 8, 2024 06:50
@ChuanqiXu9 ChuanqiXu9 marked this pull request as ready for review November 8, 2024 06:58
@bcardosolopes
Copy link
Member

Since I can't create branches to this repo so that I can't start real stacked PRs

Fixed

@bcardosolopes
Copy link
Member

bcardosolopes commented Nov 8, 2024

The practical impact the patch is, after this patch, with #1088 and a small following up fix, we can build and run all C's benchmark in SpecCPU 2017. I think it is a milestone.

Wow, this is amazing. Is that only for x86_64 or did you try other arches?

@bcardosolopes
Copy link
Member

I'll take a look after the next PR update (conflicts + stacked approach, or after the other one lands)

@ChuanqiXu9
Copy link
Member Author

The practical impact the patch is, after this patch, with #1088 and a small following up fix, we can build and run all C's benchmark in SpecCPU 2017. I think it is a milestone.

Wow, this is amazing. Is that only for x86_64 or did you try other arches?

I only tested on X86_64

@ChuanqiXu9
Copy link
Member Author

Close to create stacked PR

@ChuanqiXu9 ChuanqiXu9 closed this Nov 11, 2024
@ChuanqiXu9
Copy link
Member Author

@bcardosolopes after I rebased, I noticed the return value of datalayout.getTypeStoreSize(Ty).getFixedValue() goes to 2 from 16 for Long Double. Is this a regression?

@smeenai
Copy link
Collaborator

smeenai commented Nov 11, 2024

@bcardosolopes after I rebased, I noticed the return value of datalayout.getTypeStoreSize(Ty).getFixedValue() goes to 2 from 16 for Long Double. Is this a regression?

That might be related to #1089? That reverted a previously landed commit, and there's some discussion there.

@ChuanqiXu9
Copy link
Member Author

It looks like the case. Thanks.

@ChuanqiXu9
Copy link
Member Author

Since I can't create branches to this repo so that I can't start real stacked PRs

Fixed

Thanks. BTW, can we add a rule like the upstream that we can only create branches starts with users/.... So that we avoid we push some private branches accidently...

bcardosolopes pushed a commit that referenced this pull request Nov 13, 2024
This patch implements transformations for VAArg in X86_64 ABI **in
shape**. `In shape` means it can't work properly due to the dependent
X86_64 ABI is not robust. e.g., when we want to use VAArg with `long
double`, we need #1087.

This patch literally implement
https://github.com/llvm/llvm-project/blob/d233fedfb0de882353c348cd1ac57dab619efa6d/clang/lib/CodeGen/Targets/X86.cpp#L3015-L3240
in CIR.

There some differences due to the traditional pipeline are converting
AST to LLVM and we're transforming CIR to CIR. And also to get the ABI
Info, I moved `X86_64ABIInfo` to the header.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants