Skip to content

Runtime intrinsics: fix fpext and fptrunc behaviour on Float16/BFloat16 #57160

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

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

xal-0
Copy link
Member

@xal-0 xal-0 commented Jan 24, 2025

This makes two changes fpext and fptrunc to match the behaviour specified in their error strings:
-fpext works when converting from Float16 => Float16,
-fptrunc is prevented from truncating Float16 => Float16

Both are re-written to make it explicit what conversions are possible, and how they are done.

Closes #57130.

@xal-0
Copy link
Member Author

xal-0 commented Jan 24, 2025

I'm still not sure what the right thing to do is when doing fpext/fptrunc to/from BFloat16. Currently, it is allowed when "output bits >= input bits" holds for fpext, and when "input bits < output bits" for fptrunc. This has the weird effect of losing information on fpext:

julia> using .Core.Intrinsics
julia> using .Core: BFloat16
julia> fpext(Float16, fpext(BFloat16, Float16(12.3)))
Float16(12.31)

@xal-0 xal-0 force-pushed the fix-float16-fptrunc-fpext branch from 29628b6 to d4ad742 Compare January 24, 2025 22:04
@LilithHafner LilithHafner added maths Mathematical functions float16 labels Jan 24, 2025
@oscardssmith
Copy link
Member

oscardssmith commented Jan 24, 2025

I believe the correct behavior would be for fpext to allow going from Float16 or Bfloat16 to Float32/Float64 but not from Float16 to BFloat16 (or from Bfloat16 to Float16). I think that fpext is supposed to only allow transitions from src to dest if all possible src values are losslessly encodable in dest.

See also https://discourse.llvm.org/t/converting-between-different-but-same-sized-floating-point-types/59538 which suggests that there might not be any general understanding of what should happen here.

@xal-0
Copy link
Member Author

xal-0 commented Jan 25, 2025

LLVM seems to prohibit fpext and fptrunc with input size = output size:

llc: error: llc: fpstuff.ll:2:14: error: invalid cast opcode for cast from 'float' to 'float'
  %y = fpext float %x to float
             ^

llc: error: llc: fpstuff.ll:70:16: error: invalid cast opcode for cast from 'half' to 'bfloat'
  %y = fptrunc half %x to bfloat
               ^

llc: error: llc: fpstuff.ll:75:14: error: invalid cast opcode for cast from 'half' to 'bfloat'
  %y = fpext half %x to bfloat
             ^

@vchuravy vchuravy requested a review from maleadt January 25, 2025 09:09
Copy link
Member

@maleadt maleadt left a comment

Choose a reason for hiding this comment

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

LGTM (superficially at least, it's been a while since I looked at this code).

For BFloat16s.jl it'd be fine to reject fpext/fptrunc between f16 and bf16, it currently should go through f32 anyway.

Comment on lines +1660 to +1661
else
jl_error("fptrunc: runtime floating point intrinsics are not implemented for bit sizes other than 16, 32 and 64");
Copy link
Member

Choose a reason for hiding this comment

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

Since you are changing the permitted combination of behaviors, you will also need to update Compiler/src/tfuncs.jl and src/intrinsics.cpp to match these new rules, so that they all agree on exactly which errors are thrown and for what cases they can occur

xal-0 added 2 commits January 28, 2025 18:00
We also throw an exception when using fpext/fptrunc on integer types, because
with the addition of BFloat16, there is no unambiguous floating point format
with which to interpret the input when it is 16 bits wide.
BFloat16 tests that trigger codegen are disabled for now, pending an LLVM fix.
@xal-0
Copy link
Member Author

xal-0 commented Jan 30, 2025

I decided to leave the incorrect handling of NaNs for later, since what to do is not entirely clear (#49353).

double_to_half, float/double_to_bfloat TODO
@xal-0 xal-0 force-pushed the fix-float16-fptrunc-fpext branch from cdbda7c to 7431a86 Compare January 31, 2025 20:21
@xal-0 xal-0 force-pushed the fix-float16-fptrunc-fpext branch from 7431a86 to e2d3cd9 Compare January 31, 2025 20:30
@oscardssmith oscardssmith added compiler:codegen Generation of LLVM IR and native code compiler:effects effect analysis labels Jan 31, 2025
@xal-0 xal-0 force-pushed the fix-float16-fptrunc-fpext branch from 86d3070 to fa82f94 Compare February 3, 2025 18:17
@oscardssmith oscardssmith merged commit 67e992d into JuliaLang:master Feb 3, 2025
7 checks passed
vtjnash pushed a commit that referenced this pull request Mar 21, 2025
The `fptrunc`/`fpext` intrinsics were modified in #57160 to throw on
non-float arguments.
- The arithmetic and math float intrinsics now require all their
arguments to be floats
- `fptosi`/`fptoui` require their source to be a float
- `sitofp`/`uitofp` require their destination type to be a float

Also fixes #57384.
KristofferC pushed a commit that referenced this pull request Mar 22, 2025
The `fptrunc`/`fpext` intrinsics were modified in #57160 to throw on
non-float arguments.
- The arithmetic and math float intrinsics now require all their
arguments to be floats
- `fptosi`/`fptoui` require their source to be a float
- `sitofp`/`uitofp` require their destination type to be a float

Also fixes #57384.

(cherry picked from commit 0bcc9cd)
serenity4 pushed a commit to serenity4/julia that referenced this pull request May 1, 2025
…16 (JuliaLang#57160)

This makes two changes `fpext` and `fptrunc` to match the behaviour
specified in their error strings:
-`fpext` works when converting from Float16 => Float16,
-`fptrunc` is prevented from truncating Float16 => Float16

Both are re-written to make it explicit what conversions are possible,
and how they are done.

Closes JuliaLang#57130.

---------

Co-authored-by: Jameson Nash <[email protected]>
serenity4 pushed a commit to serenity4/julia that referenced this pull request May 1, 2025
)

The `fptrunc`/`fpext` intrinsics were modified in JuliaLang#57160 to throw on
non-float arguments.
- The arithmetic and math float intrinsics now require all their
arguments to be floats
- `fptosi`/`fptoui` require their source to be a float
- `sitofp`/`uitofp` require their destination type to be a float

Also fixes JuliaLang#57384.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code compiler:effects effect analysis float16 maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core.Intrinsics.fpext cannot extend Float16 to Float16
5 participants