float types: move copysign, abs, signum to libcore

[RalfJung]

These operations are explicitly specified to act "bitwise", i.e. they just act on the sign bit and do not even quiet signaling NaNs. We also list them as "non-arithmetic operations", and all the other non-arithmetic operations are in libcore. There's no reason to expect them to require any sort of runtime support, and from these experiments it seems like LLVM indeed compiles them in a way that does not require any sort of runtime support.

Nominating for @rust-lang/libs-api since this change takes immediate effect on stable.

Part of #50145.

[rustbot]

r? @cuviper

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

[workingjubilee]

abs does not actually require an intrinsic at all anymore... the optimization reasons are moot, in other words, in the minimum LLVM version we support... and can be redefined in terms of its bit operation, LLVM can promote such to fabs: llvm/llvm-project@5c0da5839de1

I was working on PRing such with some tests to validate that no calls to __builtin_fabs or such were generated.

[workingjubilee]

That said, we can always read the source.
The libcall lowering is here: https://github.com/llvm/llvm-project/blob/461274b81d8641eab64d494accddc81d7db8a09e/llvm/lib/CodeGen/IntrinsicLowering.cpp#L202-L452

It does not generate libcalls for fabs, but it does for fcopysign.

EDIT: There's also this .td file which describes all the runtime libcalls LLVM might lower:

https://github.com/llvm/llvm-project/blob/98723e656618345f05a8e66b18e0f5d37866e75e/llvm/include/llvm/IR/RuntimeLibcalls.def

[RalfJung]

It does not generate libcalls for fabs, but it does for fcopysign.

Then either things changed since @Urgau made their experiments or there are some other factors involved...

FWIW we could also implement copysign fairly trivially ourselves:

let sign_bit = mem::size_of::<Self>()*8 - 1;
Self::from_bits(self.abs().to_bits() | (sign.to_bits() & (1 << sign_bit))

[workingjubilee]

Then either things changed since @Urgau made their experiments or there are some other factors involved...

@RalfJung Architecture and size of the float, the latter of which has become particularly relevant for f128... I believe a lot of things that "shouldn't" generate libcalls nonetheless do for f128.

[workingjubilee]

my implementation of copysign was

        let sign = sign.abs().to_bits() ^ sign.to_bits();
        Self::from_bits(self.to_bits() | sign)

[RalfJung]

Judging from the f16/f128 comments, we do already have things in libcore that generate calls -- I see comments about missing {eq,gt,unord}tf or eqtf2, so seemingly even == or < can generate libcalls. So that shouldn't be a blocker. Also, these are unstable anyway.

Cc @tgross35

[workingjubilee]

We "just" need to make sure that compiler_builtins supports the relevant libcall.

[RalfJung]

my implementation of copysign was

If self is negative, self.to_bits() | sign will always remain negative, won't it? So this implementation doesn't seem right to me.

[workingjubilee]

Oh, you're right, I meant self.abs().to_bits() | sign

[RalfJung]

We "just" need to make sure that compiler_builtins supports the relevant libcall.

I don't see any copysign implementations there.
I was hoping this would be a quick PR... so I'll stop here, unless someone wants to make a compiler-builtins PR.

[workingjubilee]

@RalfJung If you cut this down to fabs then I'll go pick up the copysign work.

While I was planning on being extra-pedantic, I expect a compiler should never emit a libcall for fabs. Ever.

[tgross35]

fabs and copysign are in the libm repo, not compiler-builtins. Hm... libm isn't nearly as thoroughly tested as compiler-builtins. I'm working on improving this, but it is part of the reason we don't have the other math.h functions in core yet. The functions in this PR are probably low risk but is there a way to have this not be insta-stable?

+1 to what Jubilee is saying, if we can be reasonably certain that the compiler won't emit a libcall for at least f32 and f64 then I think this is more okay.

libm also doesn't have f128 support yet (also a WIP) but there isn't any reason not to just leave the fallbacks there for now.

[workingjubilee]

As-mentioned, I am absolutely pro-landing-as-is for fabs, because it won't have any trouble with that. If your compiler backend can't lower that without a libcall, throw it out!

Maybe if it legalizes the bitops to @llvm.fabs it might hypothetically do the resizing thing for f16, which is a hypothetical pessimization, but whatever, we should already support the resizing and this isn't about good code, just code that links correctly.

[workingjubilee]

Apologies for holding up the works.

I believe I was mislead by the dead rising again in a hollow mockery of the living, and will pursue turning them away upstream:

• [LLVM] Slay undead copysign code llvm/llvm-project#111269

[RalfJung]

fabs and copysign are in the libm repo, not compiler-builtins

That's odd, why would eq and similar functions be in compiler-builtins but the much simpler fabs/copysign are in libm?

[RalfJung]

I believe I was mislead by the dead rising again in a hollow mockery of the living, and will pursue turning them away upstream:

Ah, so these libcalls are likely not actually used?

I was working on PRing such with some tests to validate that no calls to __builtin_fabs or such were generated.

Maybe we should have that first before moving these things to libcore. So if you have a good idea for how to write such a test, I'll happily wait for that. :)

[tgross35]

That's odd, why would eq and similar functions be in compiler-builtins but the much simpler fabs/copysign are in libm?

It just mirrors how things are split in the C world - compiler-builtins provides the same symbols as LLVM's compiler-rt or libgcc, libm provides whatever is in math.h. We actually ship libm as a module of compiler-builtins and are probably somewhat close to making the symbols always weakly available (we actually had this a few months ago but reverted since our symbols were getting picked first, #128386).

Looking at compiler-rt it doesn't define copysign anywhere but does make use of __builtin_copysign. I assume this has to just be getting lowered to asm since I don't think clang can rely on libm being available. Maybe a good sign that it doesn't ever make this a libcall...

[workingjubilee]

Ah, so these libcalls are likely not actually used?

Yes, my understanding is that the codepath that does "libcall legalization" of intrinsics happens "last" (speaking very, very loosely) during lowering to the machine level, and thus has several possible upstream chances to "get missed". For instance, maybe LLVM notices a call to copysign can use one of the FSGNJ* instructions on RISCV (a somewhat odd set of instructions that compose two floats with one of three bitops). I couldn't find something in SelectionDAG that guarantees we never hit the switch into Intrinsic::copysign, but I am fully willing to believe one exists somewhere in the remaining 1.99 million lines of code I didn't read, and if my PR is accepted and I am wrong then we'll find out because someone's LLVM choked on it.

[cuviper]

r? libs-api

[tgross35]

I don't believe this needs libs-api review since FCP completed #131304 (comment). The new use of const_float_methods seems acceptable, especially considering that feature just went into FCP #130843 (comment).

@bors r+

[bors]

📌 Commit 2d3c08a has been approved by tgross35

It is now in the queue for this repository.

[lopopolo]

Hey folks, I recently was making changes to a no std crate and wanted to use f64::abs. I was super confused when it didn't compile in my MSRV build since the docs for the f64 primitive show that abs is stable since Rust 1.0.0.

I would definitely have appreciated some caveats in the primitive docs for functions that have conditional availability in no std crates based on Rust version!

[RalfJung]

Sadly we don't have infrastructure for that. Feel free to open an issue so we can discuss possible ways of handling this. There might even already be one; the keyword here is "path stability" vs "item stability" -- rustdoc can only communicate the latter currently.

[lopopolo]

Even free form text in the prose of the doc comment would have solved this for me.

[lopopolo]

Filed #139066 to discuss.