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

Add support for Float16, Float32, Float64 and Float128 #17

Merged
merged 12 commits into from
Jun 25, 2024

Conversation

zedar
Copy link

@zedar zedar commented Apr 10, 2024

No description provided.

@zedar zedar force-pushed the add_support_for_float16_float128 branch 2 times, most recently from 35bc8ff to 6b2a99a Compare April 11, 2024 18:56
src/context.rs Outdated
#[cfg(feature = "master")]
let float64_type = context.new_c_type(CType::Float64);
#[cfg(feature = "master")]
let float128_type = context.new_c_type(CType::Float128);
Copy link
Owner

Choose a reason for hiding this comment

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

Those types might not be available on some platforms, so it might be wrong to query these types here.

Depending on how much you want to add stuff in this PR, you have two options:

  • Do the same as detecting 128-bit integers support: we might want to change the GCC API to take a type as a parameter.
  • Only query the types in the functions like type_f16 below so that it only panics when used.

Copy link
Author

@zedar zedar Apr 12, 2024

Choose a reason for hiding this comment

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

So far I query the types only in the functions().
As the next step I will try to detect whether new types are supported.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean, just calling new_c_type() here will give an error if the type is not supported.
Here's how I detect 128-bit support with the old way of doing things.

@@ -132,8 +137,13 @@ impl<'gcc, 'tcx> BaseTypeMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
self.double_type
Copy link
Owner

Choose a reason for hiding this comment

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

Depending on what you decide to do above, you could return Float32/Float64 in those functions if you decide to add the support for checking if those types are supported.

Of course, we would keep returing Float and Double on platforms where those types are not supported.

@zedar zedar force-pushed the add_support_for_float16_float128 branch from 6b2a99a to f9c3617 Compare April 12, 2024 06:57
src/type_.rs Outdated
fn type_f16(&self) -> Type<'gcc> {
unimplemented!("f16_f128")
self.context.new_c_type(CType::Float16)
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm surprised the CI didn't complain about formatting, but it's missing an empty line between the two methods.

src/type_.rs Outdated
#[cfg(feature = "master")]
fn type_f128(&self) -> Type<'gcc> {
self.context.new_c_type(CType::Float128)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

@antoyo
Copy link
Owner

antoyo commented Apr 13, 2024

Do you want to add more stuff for this PR (like the checks if the types are supported on the target platform for at least Float32 and Float64) or do you want to merge as is (with the above formatting nitpicks fixed)?

@zedar
Copy link
Author

zedar commented Apr 14, 2024

Do you want to add more stuff for this PR (like the checks if the types are supported on the target platform for at least Float32 and Float64) or do you want to merge as is (with the above formatting nitpicks fixed)?

Yes, I plan to add checks if Float32 and Float64 are supported.

@zedar zedar force-pushed the add_support_for_float16_float128 branch from f9c3617 to 46eea99 Compare April 14, 2024 18:20
@zedar zedar force-pushed the add_support_for_float16_float128 branch from 46eea99 to 2743807 Compare April 22, 2024 18:23
@zedar zedar force-pushed the add_support_for_float16_float128 branch 5 times, most recently from d71f067 to 6280647 Compare May 3, 2024 04:48
@zedar zedar force-pushed the add_support_for_float16_float128 branch 4 times, most recently from 98af2e1 to 8920002 Compare May 13, 2024 06:02
@zedar zedar force-pushed the add_support_for_float16_float128 branch 4 times, most recently from d9dc4cb to 7004c96 Compare May 13, 2024 20:31
@zedar zedar force-pushed the add_support_for_float16_float128 branch from 850b944 to 6f3aa00 Compare May 24, 2024 06:35
Robert Zakrzewski and others added 7 commits June 21, 2024 16:12
Upgrade libgccjit.version

Limit new Floatxx types to master branch only

apply rustfmt

Make new types available only when requested

Make new types available only when requested

Check if Float16 and Float128 are supported by the target platform

Replace Float with Float32 and Double with Float64 if target dependent type is defined

Add support for Float16|32|64|128 in the builder

Fix cargo fmt errors

Update gccjit wrapper

update hash of ligccjit
…he no-f16-f128 feautes introduced incompatibility
reorder type_kind

reorder type_kind

reorder type_kind

fix

fix

fix

fix
@zedar zedar force-pushed the add_support_for_float16_float128 branch from 894f00c to 55788e4 Compare June 21, 2024 14:12
src/type_.rs Outdated
Comment on lines 141 to 152
#[cfg(feature = "master")]
fn type_f32(&self) -> Type<'gcc> {
if self.supports_f32_type {
return self.context.new_c_type(CType::Float32);
}
self.float_type
}

#[cfg(not(feature = "master"))]
fn type_f32(&self) -> Type<'gcc> {
self.float_type
}
Copy link
Owner

Choose a reason for hiding this comment

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

Would it work to do the following?

Suggested change
#[cfg(feature = "master")]
fn type_f32(&self) -> Type<'gcc> {
if self.supports_f32_type {
return self.context.new_c_type(CType::Float32);
}
self.float_type
}
#[cfg(not(feature = "master"))]
fn type_f32(&self) -> Type<'gcc> {
self.float_type
}
fn type_f32(&self) -> Type<'gcc> {
#[cfg(feature = "master")]
if self.supports_f32_type {
return self.context.new_c_type(CType::Float32);
}
self.float_type
}

src/type_.rs Outdated
4 => TypeKind::Float,
8 => TypeKind::Double,
16 => TypeKind::FP128,
_ => TypeKind::Void,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this should be unreachable!() instead?

TypeKind::Float
} else if typ.is_compatible_with(self.double_type) {
TypeKind::Double
} else if typ.is_floating_point() {
Copy link
Owner

Choose a reason for hiding this comment

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

You'll need to do this:

Suggested change
} else if typ.is_floating_point() {
}
#[cfg(feature="master")]
else if typ.is_floating_point() {

Copy link
Owner

@antoyo antoyo Jun 21, 2024

Choose a reason for hiding this comment

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

This syntax isn't supported.
Unless you can put this above the if, I guess you can revert to 2 different implementations:

#[cfg(feature="master")]
if typ.is_floating_point() {
    match typ.get_size() {
                 2 => return TypeKind::Half,
                 4 => return TypeKind::Float,
                 8 => return TypeKind::Double,
                 16 => return TypeKind::FP128,
                 _ => return TypeKind::Void,
    }
}

#[cfg(feature = "master")]
if self.supports_f32_type {
return self.context.new_c_type(CType::Float32);
}
self.float_type
}

Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to implement type_f64 as well.

@zedar zedar force-pushed the add_support_for_float16_float128 branch from 0d05848 to 2eaac23 Compare June 21, 2024 15:33
@antoyo antoyo force-pushed the add_support_for_float16_float128 branch from 55467aa to 9274c63 Compare June 25, 2024 13:00
@antoyo antoyo merged commit 70d3655 into antoyo:master Jun 25, 2024
34 checks passed
@antoyo
Copy link
Owner

antoyo commented Jun 25, 2024

Thank you so much for your work!

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.

2 participants