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.

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

Jump to bottom

[CIR][CIRGen][Builtin][Type] Support for IEEE Quad (long double) added (in CIR + Direct to LLVM) #966

Merged
merged 5 commits into from
Oct 16, 2024
Merged

[CIR][CIRGen][Builtin][Type] Support for IEEE Quad (long double) added (in CIR + Direct to LLVM) #966

merged 5 commits into from
Oct 16, 2024

Conversation

mvvsmk
Copy link
Contributor

@mvvsmk mvvsmk commented Oct 13, 2024

Addresses #931
Added type definition in CIRTypes.td, created appropriate functions for the same in CIRTypes.cpp like getPreferredAlignment, getPreferredAlignment, etc. Optionally added lowering in LowerToLLVM.cpp

This is for test case2 : (https://godbolt.org/z/8Y85148EY)
If you want me to add the test case to any perticular folder / with a certain name please let me know,

As of test case 1, rightly pointed out by @Lancern it might be an due to lack of support for int128 and is tracked by another issue.

If I missed anything do let me know :)

@mvvsmk
Support for IEEE Quad (long double) added (in CIR + Direct to LLVM)
95f6510 
Added type definition in CIRTypes.td, created appropriate functions
for the same in CIRTypes.cpp like getPreferredAlignment,
getPreferredAlignment, etc. Optionally added lowering in
LowerToLLVM.cpp
Lancern
Lancern requested changes Oct 13, 2024
View reviewed changes
Copy link
Member

@Lancern Lancern left a comment

Choose a reason for hiding this comment

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

Cool, thanks for working on this! Please add tests for the changes. The new set of tests should cover both the CIRGen part and the LLVM lowering part.

At a bare minimum, check out clang/test/CIR/CodeGen/types.c and update it to include fp128.

@mvvsmk
Added Test cases for long double (IEEE Quad)
6843f0e 
Long double gets converted to fp128 (IEEE Quad) when compiled
for a few selected architectures, hence compile and checks
added to types-IEEE-quad.c (for cir gen check), and
types-IEEE-quad.cir (for cir direct lowering) in their respective
folders.
@mvvsmk
Copy link
Contributor Author

mvvsmk commented Oct 13, 2024

Thanks for you suggestions @Lancern I have added 2 test cases one for CIR gen and one for lowering.

@mvvsmk mvvsmk marked this pull request as ready for review October 13, 2024 10:39
@mvvsmk
Cleaned unessesary comment in code
ea84170 
removed the llvm_unreachable("NYI") for IEEEquad.
Lancern
Lancern approved these changes Oct 13, 2024
View reviewed changes
Copy link
Member

@Lancern Lancern left a comment

Choose a reason for hiding this comment

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

LGTM

smeenai
smeenai reviewed Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

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

Thank you very much for this!

It seems some unrelated formatting/header include changes got pulled in. It's not a problem here, but it can sometimes make diffs harder to read, so if there's any editor configuration you can find to disable those, it might be useful for the future :)

@mvvsmk
Header changes and test case changes implimented
8e80de8 
Unused header removed from CIRTypes.cpp, combined the lowering
tests and the cir gen tests into one, with proper FILECHECK
directives.
@mvvsmk
Copy link
Contributor Author

mvvsmk commented Oct 14, 2024

@smeenai Thank you, for taking out the time to review my PR, I have incorporated the changes you suggested. :)

@mvvsmk mvvsmk requested a review from smeenai October 14, 2024 10:33
smeenai
smeenai approved these changes Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

bcardosolopes
bcardosolopes requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, inline comment follows.

@mvvsmk
Modified test case to check for CIR and LLVM IR
e096d26 
Modified test case to change prefix to check cir
to CIR and to change prefect to check llvm ir to
LLVM.
@mvvsmk
Copy link
Contributor Author

mvvsmk commented Oct 15, 2024

@bcardosolopes , Thank you for reviewing my PR, I have made the changes you requested. :)

@mvvsmk mvvsmk requested a review from bcardosolopes October 15, 2024 10:32
@smeenai smeenai merged commit 0a0c556 into llvm:main Oct 16, 2024
6 checks passed
@mvvsmk
Copy link
Contributor Author

mvvsmk commented Oct 16, 2024

Thanks guys for all the help along the way. :)

@smeenai
Copy link
Collaborator

smeenai commented Oct 16, 2024

Thank you for the patch; long double support is super valuable! We hang out on the Discord channel pretty regularly, if you're ever looking to chat about anything ClangIR-related. (You'll have to join the LLVM Discord first, https://discord.com/invite/xS7Z362)

keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
@mvvsmk @lanza
[CIR][CIRGen][Builtin][Type] Support for IEEE Quad (long double) adde…
0d894e1 
…d (in CIR + Direct to LLVM) (llvm#966)

Fixes llvm#931
Added type definition in CIRTypes.td, created appropriate functions for
the same in CIRTypes.cpp like getPreferredAlignment,
getPreferredAlignment, etc. Optionally added lowering in LowerToLLVM.cpp
@PikachuHyA PikachuHyA mentioned this pull request Nov 5, 2024
Closed
lanza pushed a commit that referenced this pull request Nov 5, 2024
@mvvsmk @lanza
[CIR][CIRGen][Builtin][Type] Support for IEEE Quad (long double) adde…
f12577e 
…d (in CIR + Direct to LLVM) (#966)

Fixes #931
Added type definition in CIRTypes.td, created appropriate functions for
the same in CIRTypes.cpp like getPreferredAlignment,
getPreferredAlignment, etc. Optionally added lowering in LowerToLLVM.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smeenai smeenai smeenai approved these changes

@bcardosolopes bcardosolopes bcardosolopes approved these changes

@Lancern Lancern Lancern approved these changes

@lanza lanza Awaiting requested review from lanza lanza is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mvvsmk @smeenai @bcardosolopes @Lancern