-
Notifications
You must be signed in to change notification settings - Fork 121
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] introduce CIR floating-point types #385
Conversation
Is there a reason to not also add support for float16 and bfloat16? |
@philnik777 Thanks for your comment. I thought there aren't any standard types corresponding to float16 and bfloat16. But seems that C++23 now supports fixed width floating-point types and there are now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this is awesome, happy to see progress in this direction, it's been a while since we want to have CIR specific float support. Some initial feedback in the comments.
Please also add new testcases to test aliases and cir to cir.
@sitio-couto because you did |
@Lancern when you update the PR, please also make sure tests pass, so reviewers can play around with it locally. I know this is a Draft, but no problem making it a first class PR already - incremental work is fine and encouraged! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lancern thanks for tackling this! It's long overdue.
I just added a suggestion and a discussion with @bcardosolopes.
✅ With the latest revision this PR passed the C/C++ code formatter. |
I met a problem trying to lower PPC fp128 type is lowered to a special |
I have asked in the MLIR channel on Discord. Seems that support for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @Lancern, more feedback for you inline. Couple of big picture comments:
- Please make this first PR as simple as possible, focus on float and double and we could go over the rest with incremental work.
- Apply @sitio-couto changes and create individual types for each floating point type.
- Remove changes (mostly formatting) that are not related to the functionality you are changing - the amount of noise makes reviewing pretty hard.
Thanks for all your comments and early reviews here! According to the early reviews, I'm going to further update this PR with the following TODOs list:
|
Rebased onto |
@Lancern thanks for the fast fixes and the work you've been putting up into the PRs. I'm currently working on an RFC to upstream clangir, so I'm a bit behind on reviews, I'll take a look into this one and others as soon as possible. |
@bcardosolopes Just take your time! IMO your review latency is already good enough and it's understandable to slow down a bit especially if you have more important thing to do first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sound like many of the changes requested haven't applied yet, I still see lots of things around that are out of the scope of this patch. Let me know once you go over all of them.
Thanks for everyone's early comments and reviews! Now I should have resolved all confirmed problems during the early review and there're no more TODOs on my list. So I believe this PR should be ready. |
4e069c6
to
79d4dc7
Compare
@lanza just rebased and this has the side effect of breaking GH PR workflow, making it impossible to review, apologies. Please rebase! |
@bcardosolopes Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience, getting closer!
Let me know when you're done applying all reviews (without conflicts) and ready for another round of review! |
@bcardosolopes Hey I should have resolved all reviews yet. The primary change is replacing the |
Rebased onto the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work, only a few nit remaining!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for putting solid work here and for your patience. LGTM
This PR adds a dedicated `cir.float` type for representing floating-point types. There are several issues linked to this PR: llvm#5,
This PR adds a dedicated `cir.float` type for representing floating-point types. There are several issues linked to this PR: #5,
This PR adds a dedicated
cir.float
type for representing floating-point types. There are several issues linked to this PR: #5, #78, and #90.The new
cir.float
type is parameterized by an enumFloatFormat
which defines the format of the floating point values. Currently 7 formats are defined:FF_Binary16
, which represents IEEE-754 binary16 format;FF_Binary32
, which represents IEEE-754 binary32 format, and corresponds to thefloat
type in C/C++;FF_Binary64
, which represents IEEE-754 binary64 format, and corresponds to to thedouble
andlong double
type in C/C++;FF_Binary128
, which represents IEEE-754 binary128 format, and corresponds to thelong double
type in C/C++.FF_X86Fp80
, which represents x86 80-bit floating point format, and also corresponds to thelong double
type in C/C++;FF_BFloat16
, which represents brain float format;FF_PpcFp128
, which represents PowerPC 128-bit floating point format. It is also known as thedouble double
format. It also corresponds to thelong double
type in C/C++.What floating-point format the
long double
type uses depends both on the implementation and on the target platform.TODOs
cir.float
and attribute#cir.float
, and replace all existing usages of builtins with the new type and attribute;Updated on Jan 8, 2024
FF_Binary16
andFF_BFloat16
formats.Updated on Jan 15, 2024
FF_Binary64E
toFF_X86Fp80
to avoid confusion.Addppc_fp128
format.