Tracking Issue for `const_mul_add`
Opened this issue · 29 comments
Feature gate: #![feature(const_mul_add)]
This is a tracking issue for making mul_add const.
Public API
impl {f16, f32, f64, f128} {
pub const fn mul_add(self, a: Self, b: Self) -> Self;
}Steps / History
(Remember to update the S-tracking-* label when checking boxes.)
- Implementation: #...
- Final comment period (FCP)1
- Stabilization PR
Unresolved Questions
- None yet.
Footnotes
Cc @rust-lang/libs-api @RalfJung: after the recent constification of rounding methods, this is the last float op supported by rustc_apfloat but not yet made const.
Labeled help wanted if anyone is interested in picking it up. The PR should look pretty similar to @ruancomelli's #141521 (move the implementation from miri to rustc_const_eval, move tests from library/coretests/tests/floats/f*.rs to library/coretests/tests/floats/mod.rs so they also get tested in const)
@kliwongan thanks for the interest! There is a PR open here already though #146735.
(I can't reassign @Qelxiros directly)
Oh no problem, I'll just try another issue for now
I see that the impl PR was merged last month. Seems like this is ready for stabilization?
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.
Under the hood this is an intrinsic which needs to be const-stabilized to complete this, so we should at least ping @rust-lang/lang. However the intrinsic just computes a pure function on scalar values so there's nothing really interesting happening on the language level.
One point that is interesting about the intrinsic is that for a long time, the underlying implementation from apfloat that we use for this was buggy (rust-lang/rustc_apfloat#11). @tgross35 @beetrees are you confident enough in the correctness of the current implementation to expose this on stable?
Given that we have *+ on float in const, I agree there's no novel lang concerns here. (Speaking for me, not an agreed team position. And as Ralf said, it needs a sufficiently-confident implementation, but that's a libs/compiler question, not a lang one.)
Note that mul_add cannot be defined in terms of *+, since mul_add is defined to not do any rounding of the intermediate result.
Yup, totally get that, I just mean that there's no new NAN questions or new table-maker dilemna issues with mul_add that don't already exist for *+, as far as I could come up with.
Under the hood this is an intrinsic which needs to be const-stabilized to complete this, so we should at least...
In the spirit that it's often easier to follow the process rather than to ensure everyone sees the discussion about making an exception, I'd probably ask for the stabilization PR for this to be nominated for lang. Given the discussion here, we'll mark it as an I-lang-easy-decision. It'll go to the top of the agenda, and we'll likely waive it through.
(The stabilization PR can be put up while the libs-api pFCP/FCP is still ongoing.)
@rfcbot fcp cancel
@BurntSushi proposal cancelled.
@rfcbot merge
Team member @BurntSushi has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.
(I ticked @Amanieu's box, since it was previously ticked.)
On the lang side, we do our FCPs on the stabilization PR. It's actually OK for this FCP on the tracking issue to be libs-api-only. Where we'd want to see it come before us, on lang, is on the stabilization PR that leans on the intrinsic.
I created a stabilization PR if you'd like to move the FCP there #148052. However,
One point that is interesting about the intrinsic is that for a long time, the underlying implementation from apfloat that we use for this was buggy (rust-lang/rustc_apfloat#11). @tgross35 @beetrees are you confident enough in the correctness of the current implementation to expose this on stable?
From a quick check I tried, I'm getting some iffy results e.g. fma(0.0000015, 15.36, 0.01563) for f16. I need to verify my methodology though.
@tgross35 f16 isn't stable yet, right? That should be added to open issues on f16 stabilization.
If there are any iffy results for stable types, though, then that's concerning. That said, aren't these intrinsics already exposed, just not for const? Or are we using a new impl for const that we weren't using for non-const?
The const implementation of this intrinsic is one uniform function that handles all sizes. So if f16 is wrong, chances are the other types have the same bug -- f16 is just much easier to exhaustively test.
const uses a soft-float implementation in rustc_apfloat that is so far not exposed at all (except via Miri). This matches what we do for other float operations in const -- we never use the float types of the host we are compiling on, it's always a fully IEEE-compliant deterministic soft-float implementation.
Given that @rust-lang/lang doesn't need to be on this FCP, I'm going to cancel the FCP and re-start it for just libs-api, and re-check all the boxes that were previously checked.
@rfcbot cancel
@joshtriplett proposal cancelled.
Sigh, GitHub...
@rfcbot merge
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
🔔 This is now entering its final comment period, as per the review above. 🔔
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.