intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics

[jedbrown]

Add intrinsics fmuladd{f32,f64}. This computes (a * b) + c, to be fused if the code generator determines that (i) the target instruction set has support for a fused operation, and (ii) that the fused operation is more efficient than the equivalent, separate pair of mul and add instructions.

https://llvm.org/docs/LangRef.html#llvm-fmuladd-intrinsic

The codegen_cranelift uses the fma function from libc, which is a correct implementation, but without the desired performance semantic. I think this requires an update to cranelift to expose a suitable instruction in its IR.

I have not tested with codegen_gcc, but it should behave the same way (using fma from libc).

---

This topic has been discussed a few times on Zulip and was suggested, for example, by @workingjubilee in Effect of fma disabled.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jhpratt (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[jhpratt]

Given that this is dealing primarily with intrinsics, I think this is T-compiler land? In any situation this isn't my area of knowledge.

r? compiler

[workingjubilee]

T-compiler? hmm, in implementation, but

hey @rust-lang/opsem we're discussing adding nondeterministic float rounding.

[RalfJung]

Is this a good opportunity to use our fallback bodies, with a fallback implementation of a * b + c? Though looking at what you did for GCC and cranelift, maybe the fallback should be to call the fused intrinsic?

Miri however should probably roll some dice to make both behaviors observable.

[jedbrown]

I don't have strong rationale for choosing the fused versus separate, but it was the simplest to implement and presumably to change if/when Cranelift adds a corresponding intrinsic.

Could you give a pointer for how/where to implement the "make both behaviors observable" in Miri?

[RalfJung]

The miri implementation would go in src/tools/miri/src/intrinsics/mod.rs. There you can use this.machine.rng.get_mut() to get access to an RNG that you can use to decide whether to use the fused implementation or the double-rounding implementation. That way, if the function is called many times in Miri, both behaviors will be observable.

[jedbrown]

I implemented this and added a test in the spirit of is_val_statically_known in src/tools/miri/tests/pass/intrinsics/intrinsics.rs (looping 50 times to observe both outcomes). Let me know if there is a better way to ensure that.

[RalfJung]

Yeah full non-det is kind of what I would expect. The intrinsic can just say "nondeterministic" but the public function should probably spell this out in its docs.

[workingjubilee]

@jedbrown So, T-libs-api gets to decide whether the name on f32/f64 is good, basically. It might be best to land the intrinsic and then PR the public safe API.

[jedbrown]

@jedbrown So, T-libs-api gets to decide whether the name on f32/f64 is good, basically. It might be best to land the intrinsic and then PR the public safe API.

Should I update this PR to only handle the intrinsic?

[workingjubilee]

@jedbrown up to you but it would mean that only the people in charge of the compiler/opsem decide whether it's merged so a shorter path to getting this PR over and done with and then the bikeshedding, if any, can be on its own PR

[clarfonthey]

Not to further delay merging, but given how this fits more along the line of the existing wrapping methods in the sense that it might be worth eventually adding a Fast wrapper type, perhaps fast_mul_add would be a better name?

Imagining there are many other operations that could get this treatment, like hypot.

[saethlin]

The point of such types is to provide operator overloads that make using the type ergonomic. You can't do that with our _fast semantics because all the _fast operations are unsafe. You'd need an unsafe version of the operators. So existing fast float libraries just ignore this and are unsound or have uselessly broad preconditions such as https://docs.rs/fast-floats/0.2.0/fast_floats/struct.Fast.html#safety.

That's why I proposed the _algebraic intrinsics. Those make it possible to write a usable wrapper. You could of course RFC unsafe operator overloads to make the _fast version technically possible to write a sound API over, though I submit that in practice it's absurd to prove preconditions like "this float will never be NaN".

[clarfonthey]

I guess that the issue is that "fast" is too overloaded with existing "fast float" semantics, although it really is the best term to use here. It's the only way to definitively summarise that, between fused multiplication and simple multiply-add, it chooses whatever is the fastest option, not necessarily the accurate one. Similarly, we could use it for hypot to indicate that the simpler approach that sacrifices accuracy is allowed too.

"Approximate" doesn't necessarily summarise this, since while it points out you're allowing approximations, it could imply it's worse than what you'd expect.

"Algebraic" honestly doesn't do it for me. To me, algebraic means specifically algebraic numbers and alludes to stuff like taylor series being used to compute the result, which is not what you're doing here. I get that you mean "algebraic simplification" but the key word there is simplification, not algebraic.

On that end, "simple" might work as a compromise here, but I dunno. I really think that we could just use the term "fast" here to mean "accuracy may be sacrificed, but soundness won't be" and if we want to add in potentially-unsound versions, we could use the term "unsafe" or "unchecked" instead of fast.

[CAD97]

I just want to note that this naming "clashing" with -ffast-math style "fast" isn't fully wrong per say either, as the contraction from separate mul and add to fused mul/add is exactly the kind of optimization which the safe subset of fast-math flags are enabling.

It seems very unfortunate to tie this one case to the more general bikeshed that is exposing fast math semantics more generally, but it is exactly the same naming question, just with the benefit of the safe subset (that doesn't assume that all is finite) much more clear than with each of the primitive ops separate.

Given the precedent of unchecked_op on the integer types meaning assume no wrapping, I can support unchecked_op meaning assume no non-finite for the floating point types. There are three categories of current fast-math opts:

• finite
  • assume no NaN (nnan; NaN becomes poison)
  • assume no Inf (ninf; ±Inf becomes poison)
• algebraic
  • no signed zeros (nsz; zero always has ndet sign)
  • allow reciprocal (arcp; minor ndet impact on precision)
  • allow contraction (contract; minor ndet impact on precision)
  • allow reassociation (reassoc; may ndet "dramatically" change results)
• approx
  • allow function approximation (afn; may ndet significantly impact precision)

and "fast" means all of them. This function only wants contraction and only of the one mul/add operation; applying any more of these flags would not be correct as per the PR description.

Other potentially useful labels: weak, inexact, lenient, relaxed

---

This restates a lot of the information but isn't all that helpful in resolving this specific naming. I'd suggest landing the intrinsics first and adding the public function veneer in a follow up PR as I don't personally expect this bikeshed to get its paint any time soon.

[clarfonthey]

I'd suggest landing the intrinsics first and adding the public function veneer in a follow up PR as I don't personally expect this bikeshed to get its paint any time soon.

Agreed! I hadn't even realised this was just the intrinsic and not the method version. I'm fine with this staying unstable under whatever name it gets as long as it's documented.

[nnethercote]

lol, I don't think randomly rolling for a review on a PR that is 4.5 months old and has dozens of comments from people with r+ permissions is a good idea. I know very little about floating point arithmetic, so I will pass this on to one of the people who does know about that topic and has already weighed in:

r? @saethlin

[saethlin]

Oh my, brave of you to guess that I know much about floating point arithmetic. I'm learning most of this on the fly, I just have some experience on the receiving end of fast-math stuff and it was very valuable to me then.

@jedbrown You've added backend support for f16 and f128 intrinsics but then not wired them up to library/core/src/intrinsics.rs; is that because there's no hardware support for f16 and f128 fma yet?

[saethlin]

Regardless of answer to that question, I think this is good to land. These intrinsics should be useful in the community for building out libraries that hopefully can drive proper Rust support for fast floats.

@bors r+

[bors]

📌 Commit c0744ff has been approved by saethlin

It is now in the queue for this repository.

[RalfJung]

Ah yeah seems like mostly the f16 / f128 intrinsics are declared in libcore, so we should declare these as well to make sure the docs are in sync with the codegen backend. But that can be a follow-up PR.

[bors]

⌛ Testing commit c0744ff with merge 8f1fa67...

[bors]

💔 Test failed - checks-actions

[jedbrown]

I rebased and updated this PR to include the f16 and f128 intrinsics (thanks @saethlin), and I can use them when I enable the corresponding features, with or without hardware support (e.g., vfmadd213sh are issued for f16 with target-cpu=sapphirerapids).

I also reduced the arguments for the test by one digit because it was just tripping f32 machine epsilon so I expect CI to pass this time.

[saethlin]

@bors r+

[bors]

📌 Commit 0d8a978 has been approved by saethlin

It is now in the queue for this repository.