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

Presence of f16 in signatures causes missing symbols on PPC #655

Closed
tgross35 opened this issue Aug 3, 2024 · 2 comments
Closed

Presence of f16 in signatures causes missing symbols on PPC #655

tgross35 opened this issue Aug 3, 2024 · 2 comments
Labels
llvm This issue needs to be fixed in LLVM

Comments

@tgross35
Copy link
Contributor

tgross35 commented Aug 3, 2024

It seems like some function signatures make PowerPC targets require __gnu_f2h_ieee or __gnu_h2f_ieee even when they wouldn't be used. For example, attempting to build this test function:

#[test]
fn demo() {
    fuzz_float(N, |x: f32|
        let _tmp0: f16 = (|_x: f32| todo!())(x);
    });
}

gives a linker error looking for __gnu_f2h_ieee. Replacing the inner line with let _tmp0: f16 = todo!(); makes them go away. It seems like this is causing CI failures like https://github.com/rust-lang/compiler-builtins/actions/runs/10225081035/job/28293642361?pr=652 since we no longer build the C version.

I'll disable the tests for now, just making this issue to remember to re-enable them once our version of the symbols is available again.

Checked on powerpc64-unknown-linux-gnu with the 2024-08-02 nightly.
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
@tgross35
Update float conversion tests
aa0135b 
Since there are more platforms that do not have symbols present, we need
to use `rustc_apfloat` for more conversion tests. Make use of the
fallback like other tests, and refactor so each test gets its own
function.

Previously we were testing both apfloat and system conversion methods
when possible. This changes to only test one or the other, depending on
whether or not the system version is available. This seems reasonable
because it is consistent with all other tests, but we should consider
updating all tests to check both at some point.

This also includes an adjustment of PowerPC configuration to account for
the linking errors at [1].

[1]: rust-lang#655
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
@tgross35
Update float conversion tests
869c8ce 
Since there are more platforms that do not have symbols present, we need
to use `rustc_apfloat` for more conversion tests. Make use of the
fallback like other tests, and refactor so each test gets its own
function.

Previously we were testing both apfloat and system conversion methods
when possible. This changes to only test one or the other, depending on
whether or not the system version is available. This seems reasonable
because it is consistent with all other tests, but we should consider
updating all tests to check both at some point.

This also includes an adjustment of PowerPC configuration to account for
the linking errors at [1].

[1]: rust-lang#655
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
@tgross35
Update float conversion tests
91b8469 
Since there are more platforms that do not have symbols present, we need
to use `rustc_apfloat` for more conversion tests. Make use of the
fallback like other tests, and refactor so each test gets its own
function.

Previously we were testing both apfloat and system conversion methods
when possible. This changes to only test one or the other, depending on
whether or not the system version is available. This seems reasonable
because it is consistent with all other tests, but we should consider
updating all tests to check both at some point.

This also includes an adjustment of PowerPC configuration to account for
the linking errors at [1].

[1]: rust-lang#655
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
@tgross35
Update float conversion tests
4872d5e 
Since there are more platforms that do not have symbols present, we need
to use `rustc_apfloat` for more conversion tests. Make use of the
fallback like other tests, and refactor so each test gets its own
function.

Previously we were testing both apfloat and system conversion methods
when possible. This changes to only test one or the other, depending on
whether or not the system version is available. This seems reasonable
because it is consistent with all other tests, but we should consider
updating all tests to check both at some point.

This also includes an adjustment of PowerPC configuration to account for
the linking errors at [1].

[1]: rust-lang#655
tgross35 added a commit to tgross35/compiler-builtins that referenced this issue Aug 3, 2024
@tgross35
Update float conversion tests
c6f95d7 
Since there are more platforms that do not have symbols present, we need
to use `rustc_apfloat` for more conversion tests. Make use of the
fallback like other tests, and refactor so each test gets its own
function.

Previously we were testing both apfloat and system conversion methods
when possible. This changes to only test one or the other, depending on
whether or not the system version is available. This seems reasonable
because it is consistent with all other tests, but we should consider
updating all tests to check both at some point.

This also includes an adjustment of PowerPC configuration to account for
the linking errors at [1].

[1]: rust-lang#655
@beetrees
Copy link
Contributor

beetrees commented Aug 3, 2024

This is caused by llvm/llvm-project#97981. On targets that use LLVM's miscompilation-inducing TypePromoteFloat f16 legalisation, LLVM defaults to passing/returning f16s as f32s unless the target says otherwise, meaning that casts are inserted even when no casts or f16 operations are present in the LLVM IR. A correct TypeSoftPromoteHalf lowering has been implemented that defaults to passing/returning f16 the same way as i16 but as TypePromoteFloat remains the default many backends are still using it.

@tgross35
Copy link
Contributor Author

Closing this since I don't think there is anything to do here, thanks for the extra info.

@tgross35 tgross35 added the llvm This issue needs to be fixed in LLVM label Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
llvm This issue needs to be fixed in LLVM
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tgross35 @beetrees