Add mul_add_relaxed methods for floating-point types

[landsharkiest]

View all comments

Implements mul_add_relaxed for f16, f32, f64, and f128, which computes (self * a) + b with relaxed precision semantics. Unlike mul_add which guarantees a fused operation, this variant allows the compiler to choose between fused or separate operations based on target performance.

This fills the gap between the precision-guaranteed mul_add and the fully-optimizable algebraic operators, providing target-specific optimization while maintaining reasonable floating-point semantics.

Tracking issue: #151770

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

[RalfJung]

@landsharkiest friendly reminder that this PR is waiting for you to react to the reviewer's feedback and to fix CI. :)
if you no longer have time to work on this, that's fine -- just let us know so the task can be picked up by someone else.

[folkertdev]

Another thing, please update the doc comment of the intrinsics to point to the public methods, like here

/// The stabilized version of this intrinsic is 
/// [`f128::mul_add`](../../std/primitive.f128.html#method.mul_add) 

rust/library/core/src/intrinsics/mod.rs

Lines 1459 to 1480 in a6050b7

	/// Returns `a * b + c` for `f128` values.
	///
	/// The stabilized version of this intrinsic is
	/// [`f128::mul_add`](../../std/primitive.f128.html#method.mul_add)
	#[rustc_intrinsic_const_stable_indirect]
	#[rustc_intrinsic]
	#[rustc_nounwind]
	pub const fn fmaf128(a: f128, b: f128, c: f128) -> f128;
	/// Returns `a * b + c` for `f16` values, non-deterministically executing
	/// either a fused multiply-add or two operations with rounding of the
	/// intermediate result.
	///
	/// The operation is fused if the code generator determines that target
	/// instruction set has support for a fused operation, and that the fused
	/// operation is more efficient than the equivalent, separate pair of mul
	/// and add instructions. It is unspecified whether or not a fused operation
	/// is selected, and that may depend on optimization level and context, for
	/// example.
	#[rustc_intrinsic]
	#[rustc_nounwind]
	pub const fn fmuladdf16(a: f16, b: f16, c: f16) -> f16;

We use the prasing of "stabilized" here even if the methods are not actually stable.

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[landsharkiest]

@landsharkiest friendly reminder that this PR is waiting for you to react to the reviewer's feedback and to fix CI. :) if you no longer have time to work on this, that's fine -- just let us know so the task can be picked up by someone else.

Sorry got caught up in school and job searching, will finally put this away! :)

[landsharkiest]

@rustbot ready

[Mark-Simulacrum]

Not necessarily looking for code changes but would like some clarification.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Mark-Simulacrum]

Hm, this seems suspicious.

I suspect this might be Claude-generated though, I don't see the same pattern repeated elsewhere (e.g., in f16 just below). @landsharkiest, is there some difference between f16/f128 miri behavior?

If this is needed I'll poke Miri folks to figure out how we can avoid it being needed, in the meantime I would drop miri from the cfg here. The right way to express this implication is for Miri to set target_has_reliable (if it's reliable) IMO.

View changes since the review

[RalfJung]

target_has_reliable_f128 is always set in Miri, but target_has_reliable_f128_math is not since that also guards some operations Miri does not support.

Unfortunately target_has_reliable_f128_math guards a wide range of things, from stuff Miri can trivially support via softfloats (like mul_add) to things Miri cannot support because f128 isn't reliable on many of our targets (like pow, sin, log, etc).

[Mark-Simulacrum]

Hm, interesting. Maybe that suggests we need f128_simple_math? :)

Does the f16 situation differ for some reason (e.g., possible to add tables for literally all values?)?

If we think this is the right thing to do happy to r+ as-is though.

[RalfJung]

For f16 its similar. All the hosts that we build Miri on support it well enough that we can at least link pow, sin and friends without causing issues, and we set target_has_reliable_f16_math if the host has that flag. But actually for operations that we implement via softfloats we'd want target_has_reliable_f16_math to be unconditionally true, it's only the transcendental operations where hosts matter.

However it's probably not worth splitting up the cfg flag just for Miri.

Tons of existing tests have cfg(any(miri, target_has_reliable_f128_math)) so the new one here is consistent.

For f16 tons of tests actually use #[cfg(target_has_reliable_f16)] { which contradicts the intent of that flag as it was explained to me... 🤷 .

[RalfJung]

which contradicts the intent of that flag as it was explained to me

Also FWIW both f16 and f128 guard e.g. the tests for recip (1.0 / self) with the has_reliable flag, not the has_reliable_math flag, despite @tgross35 explaining to me that has_reliable is just for the ABI and bit-level operations (maybe including abs and copysign?) and has_reliable_math is for anything that needs actual math, rounding, or anything like that. For other operations (e.g. max, floor), f16 uses has_reliable but f128 uses has_reliable_math. So it's all quite inconsistent and messy I think.

[tgross35]

Linking discussion #t-compiler > has_reliable_{f16,f128} vs has_reliable_{f16,f128}_math

[Mark-Simulacrum]

@bors squash msg="Add mul_add_relaxed methods for floating-point types"

[rust-bors]

🔨 2 commits were squashed into 3b7025f.

[rustbot]

Any special-casing of Miri in the standard library requires review.

cc @rust-lang/miri

[Mark-Simulacrum]

@bors r+ rollup

[rust-bors]

📌 Commit 3b7025f has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #159573 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#159573), which was unapproved.

View changes since this unapproval

[folkertdev]

you can now make this

Suggested change

	assert!(r == 9.020562075079397e-19 || r == 1.734723475976807e-18);
	assert_matches!(r, 9.020562075079397e-19 | 1.734723475976807e-18);

and that should not just print that it failed, but also show the value that didn't match.

View changes since the review