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

Simplify u128->f32 casts thanks to LLVM r334777 #51872

Closed
2 tasks
hanna-kruppe opened this issue Jun 28, 2018 · 3 comments · Fixed by #67328
Closed
2 tasks

Simplify u128->f32 casts thanks to LLVM r334777 #51872

hanna-kruppe opened this issue Jun 28, 2018 · 3 comments · Fixed by #67328
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

This issue tracks two improvements we can make to our codegen after some LLVM updates, to avoid them being buried in a closed issue.

As @nikic pointed out, https://reviews.llvm.org/D47807 defines overflow in u128->f32 casts as resulting in infinity. With that patch, we don't need to manually emit the "is the u128 >= f32::MAX + 0.5 ULP?" check as we currently do and can go back to just emitting a simple uitofp (as we have always done for {u8,u16,u32,u64} -> float casts). However, because of our policy of supporting older LLVM versions, there are actually two steps we can take here at different times:

  • At any time we can backport the patch to our LLVM fork, and skip emitting the check only if compiling with our fork
  • Once we drop support for all LLVM versions that don't have this patch (if I'm not mistaken, it would first be released in LLVM 7), remove our special handling of u128->f32 casts altogether

The latter is honestly the more important step. I doubt there is any significant code size or performance impact from the extra check, but it would be nice to get rid of the tricky code in rustc that is required to implement it.

@kennytm kennytm added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2018
@bjorn3
Copy link
Member

bjorn3 commented Sep 7, 2019

Is it already defined on the lowest supported LLVM version?

@hanna-kruppe
Copy link
Contributor Author

It's in LLVM 7 but we still support LLVM 6 right now.

@hanna-kruppe
Copy link
Contributor Author

#66973 bumps our minimum LLVM version to 7 so once that's landed we can do this.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 17, 2019
…atthewjasper

Remove now-redundant range check on u128 -> f32 casts

This code was added to avoid UB in LLVM 6 and earlier, but we no longer support those LLVM versions.
Since https://reviews.llvm.org/D47807 (released in LLVM 7), uitofp does exactly what we need.

Closes rust-lang#51872
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 19, 2019
…atthewjasper

Remove now-redundant range check on u128 -> f32 casts

This code was added to avoid UB in LLVM 6 and earlier, but we no longer support those LLVM versions.
Since https://reviews.llvm.org/D47807 (released in LLVM 7), uitofp does exactly what we need.

Closes rust-lang#51872
Centril added a commit to Centril/rust that referenced this issue Dec 19, 2019
…atthewjasper

Remove now-redundant range check on u128 -> f32 casts

This code was added to avoid UB in LLVM 6 and earlier, but we no longer support those LLVM versions.
Since https://reviews.llvm.org/D47807 (released in LLVM 7), uitofp does exactly what we need.

Closes rust-lang#51872
Centril added a commit to Centril/rust that referenced this issue Dec 20, 2019
…atthewjasper

Remove now-redundant range check on u128 -> f32 casts

This code was added to avoid UB in LLVM 6 and earlier, but we no longer support those LLVM versions.
Since https://reviews.llvm.org/D47807 (released in LLVM 7), uitofp does exactly what we need.

Closes rust-lang#51872
Centril added a commit to Centril/rust that referenced this issue Dec 20, 2019
…atthewjasper

Remove now-redundant range check on u128 -> f32 casts

This code was added to avoid UB in LLVM 6 and earlier, but we no longer support those LLVM versions.
Since https://reviews.llvm.org/D47807 (released in LLVM 7), uitofp does exactly what we need.

Closes rust-lang#51872
@bors bors closed this as completed in 6ad0b55 Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants