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 f16 and f128 Rust and C support #54

Merged
merged 3 commits into from
Jul 7, 2024
Merged

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented Jul 6, 2024

Closes #16

Copy link
Owner

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Wow, awesome work!

Comment on lines +75 to +79
PrimitiveTy::F16 => write!(
f,
"(((union {{ uint16_t bits; _Float16 value; }}){{ .bits = {} }}).value)",
val.generate_u16()
)?,
Copy link
Owner

Choose a reason for hiding this comment

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

i hate/love it

@Gankra
Copy link
Owner

Gankra commented Jul 6, 2024

I'm extremely confused by why a random macos test is failing for you...

@Gankra
Copy link
Owner

Gankra commented Jul 6, 2024

Oh! I was getting false-flagged by an unrelated test emitting a million warnings.

Whatever the default compiler is on arm64 macos does not accept the existence of __float128:

target/generated_impls/cc/f128_conv_c_repr_c_cc_caller.c:42:5: error: __float128 is not supported on this target

@Gankra
Copy link
Owner

Gankra commented Jul 6, 2024

You can address this in one of two ways:

Comment on lines +84 to +87
write!(
f,
"(((union {{ __uint128_t bits; __float128 value; }}){{ .bits = ((__uint128_t){lower:#X}ull) | (((__uint128_t){higher:#X}ull) << 64) }}).value)"
)?
Copy link
Owner

@Gankra Gankra Jul 6, 2024

Choose a reason for hiding this comment

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

As funny as this is, technically it's more correct to do char* crimes (cuz accessing the wrong member of a union is technically UB sometimes (might be C++ only but egh))

Someone suggested this wonderful abomination:

*(__float128*)memcpy(&(__float128){}, (char[]){ /* bytes here */ }, sizeof(__float128));

Copy link
Owner

Choose a reason for hiding this comment

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

(not a blocker)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Union type punning is fine in C, but not in (standard) C++, so the union implementation should be fine unless this code gets compiled as C++ (which I don't think it does?). This Stack Overflow answer appears to be a good summary.

Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't yet, but it seems like a natural evolution of the backend to support both (#31), since 95% of the code would be shared (i think). But we can cross that bridge when we want to do that.

@Gankra
Copy link
Owner

Gankra commented Jul 6, 2024

Ok reran some of the tests, and at least with the default github CC's on these platforms:

  • on linux, _Float16 is unsupported
  • on macos, __float128 is unsupported
  • on windows, both are unsupported

@beetrees
Copy link
Contributor Author

beetrees commented Jul 6, 2024

So f16 and f128 support in C is... variable. Latest documentation is: GCC for both types, Clang for _Float16 (I'm not sure if/where Clang's support for __float128 is documented). However the documentation does not always appear to be complete (e.g. on compiler explorer, GCC RISC-V appears to support _Float16 despite not being mentioned in the docs. MSVC just doesn't support either f16 or f128 AFAIK.

In terms of the platforms tested in CI:

  • x86-64 Linux supports both _Float16 and __float128 in both GCC and Clang. However, the default C compiler used in CI, GCC 11, is too old to support _Float16 on x86_64 Linux: support was added in GCC 12.
  • AArch64 MacOS uses Clang and supports _Float16 but not __float128.
  • x86-64 Windows:
    • MSVC (the default C compiler) doesn't support either type.
    • GCC (MinGW) and Clang with the -gnu target support both types.
    • Clang with the -msvc target supports _Float16 but not __float128.

Further complicating matters:

  • The standard way of using f128 is _Float128, however Clang does not support it yet (GCC does). The non-standard __float128 is supported by Clang and GCC. However, when checking on compiler explorer I've discovered that latest RISC-V GCC supports _Float128 but not __float128: I don't know if any other platforms do this. As far as I know, using _Float128 for GCC and __float128 for Clang should work everywhere that the types are supported, so I'll do that.
  • 32-bit ARM GCC requires -mfp16-format=ieee to be passed on the command line to enable _Float16 support.

Given the target and compiler version dependant nature for support for these types, I wonder if it would be better to just check the compiler stderr for <type> is not supported on this target (Clang) or '<type>' is not supported on this target (GCC) and skip the test if stderr contains such a message. This would have the advantage of only skipping/ignoring the result of the test if the compiler doesn't support the type.

@beetrees
Copy link
Contributor Author

beetrees commented Jul 6, 2024

Checking the compiler output will require deeper changes as currently the compiler output is not captured by abi-cafe. For now, I've hardcoded a list of platforms that GCC and Clang support for each type.

@beetrees
Copy link
Contributor Author

beetrees commented Jul 6, 2024

I've ignored the result of the test that fails due to CI GCC being too old. An alternative would be to increase the GCC version used in CI: according to https://github.com/actions/runner-images/blob/ubuntu22/20240630.1/images/ubuntu/Ubuntu2204-Readme.md, GCC 12 is available.

@Gankra
Copy link
Owner

Gankra commented Jul 7, 2024

Oh, awesome work!

I agree we should ideally (in the future) have better info on the compiler we're interacting with, hmm...

@beetrees
Copy link
Contributor Author

beetrees commented Jul 7, 2024

As far as I can tell CI has failed on MacOS due to the compiler being misidentified as being GCC when it is Clang. I believe this will be fixed when cc-rs is updated (cc #49), but for now I've added a !cfg!(target_vendor = "apple") to the GCC match arm. I don't know whether GCC actually support f128 on MacOS (I'd guess it probably doesn't since Clang doesn't); regardless as Apple don't support it I don't think there's a standard ABI (although the general AArch64 ABI does support f128 so I'd guess GCC would just use that like LLVM/Rust does).

@Gankra
Copy link
Owner

Gankra commented Jul 7, 2024

Works for me! We can refine this logic in followups as desired.

Thanks so much!!!!

@Gankra Gankra merged commit 03b552f into Gankra:main Jul 7, 2024
16 checks passed
@beetrees beetrees deleted the f16-f128 branch July 7, 2024 09:09
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.

Test for f128
2 participants