Skip to content

Conversation

bjoernager
Copy link
Contributor

@bjoernager bjoernager commented Apr 12, 2025

Tracking issue: #136469

This PR makes the unstable algebraic_add, algebraic_sub, algebraic_mul, algebraic_div, and algebraic_rem methods in f16, f32, f64, and f128 into const fn items.

Please note that there hasn't yet been a definitive decision on the exact semantics of these functions being const fn. But that should not block any non-const stabilisations.

Also note that a cleaner solution would be to make the intrinsics in core::intrinsics into const fn as well. This PR is merely a temporary solution, as a I am not familiar with implementing compiler built-ins.

@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 12, 2025
@bjoernager
Copy link
Contributor Author

@rustbot label +T-libs-api -T-libs

r? libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 12, 2025
@rustbot rustbot assigned BurntSushi and unassigned joboet Apr 12, 2025
@rust-log-analyzer

This comment has been minimized.

@bjoernager bjoernager force-pushed the const-float-algebraic branch from c61eb67 to 345c1e9 Compare April 12, 2025 14:08
@RalfJung
Copy link
Member

RalfJung commented Apr 12, 2025

Thanks for the PR! However, this is the wrong approach. Please don't use const_eval_select.

Instead, please

  • Mark the intrinsics being called here as const fn.
  • Add implementations of these intrinsics in compiler/rustc_const_eval/src/interpret/intrinsics.rs. You can copy the existing implementation in src/tools/miri/src/intrinsics/mod.rs, which should then be removed. This is a bit non-trivial since that implementation adds extra randomness, so you'll have to add a new machine hook in compiler/rustc_const_eval/src/interpret/machine.rs to make this randomness happen only with Miri.

It would be nicer if you could just write a fallback implementation for the intrinsic, but that has a very generic type so we can't make use of the fact that these are floats... and we can't (yet) use traits in const fn either.

This PR is merely a temporary solution, as a I am not familiar with implementing compiler built-ins.

I'm afraid I don't think we should land an improper solution if we know what the proper solution should look like. Such a "temporary" solution is bound to stay around for years.

@bjoernager
Copy link
Contributor Author

I'm afraid I don't think we should land an improper solution if we know what the proper solution should look like.

I would've maybe argued that it might not be the most important for now, given that the feature is unstable. But it is still a valid concern. And I definitely agree that this implementation is hacky.

Anyhow, thank you for the mentoring. I guess this is opportune for me to work with the compiler for once. I will look into it. :)

... closing this PR in the meantime.

@bjoernager bjoernager closed this Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants