Re: [musl] [PATCH] Properly simplified nextafter()

[Rich Felker <dalias@libc.org>]

On Wed, Aug 11, 2021 at 12:53:37AM +0200, Stefan Kanthak wrote:
> Szabolcs Nagy <nsz@port70.net> wrote:
> 
> >* Stefan Kanthak <stefan.kanthak@nexgo.de> [2021-08-10 08:23:46 +0200]:
> >> <https://git.musl-libc.org/cgit/musl/plain/src/math/nextafter.c>
> >> has quite some superfluous statements:
> >> 
> >> 1. there's absolutely no need for 2 uint64_t holding |x| and |y|;
> >> 2. IEEE-754 specifies -0.0 == +0.0, so (x == y) is equivalent to
> >>    (ax == 0) && (ay == 0): the latter 2 tests can be removed;
> > 
> > you replaced 4 int cmps with 4 float cmps (among other things).
> 
> and hinted that the result of the second pair of comparisions is
> already known from the first pair.
> 
> > it's target dependent if float compares are fast or not.
> 
> It's also target dependent whether the floating-point registers
> can be accessed by integer instructions, or need to be copied:
> some win, some loose!
> Just let the compiler/optimizer do its job!

The values have been copied already to perform isnan, so continuing to
access them does not incur any further cost.

> > (the i386 machine where i originally tested this preferred int
> > cmp and float cmp was very slow in the subnormal range and
> > iirc it also raises the non-standard input denormal exception,
> > which is fine i guess.
> 
> This exception resp. the (sticky) flag is explicitly raised/set
> in the part following the patch.
> 
> > of course soft float abis much prefer int cmp so your code is
> > likely much slower and bigger there).
> 
> 0. Doesn't musl provide target specific routines for targets with
>    soft FP?

No, quite the opposite. Targets with hard fp and native insns for
particular ops have target-specific versions, but in general musl
strongly prefers use of common implementation across all targets when
there is not an obvious [nearly-]single-insn candidate for a
specialized version.

> 1. If not: the compiler knows the target ABI and SHOULD generate
>    the proper integer comparisions there.

Here it would require the compiler to recognize that the nan case was
already ruled out, and to special-case ±0 comparison on the
representation. Of course this is possible in theory, but it's almost
surely not happening now or any time soon. I'm pretty sure soft float
targets just end up calling the libgcc function for floating point
comparison if you do that.

Now, is the difference something actually worth caring about? I'm not
sure. I'm not particularly for or against this change to begin with,
but I do want changes to be well-motivated in the sense of improving
something in some measurable way. Otherwise they add to the review
burden (not just for me but for anyone who's following changes and
trying to verify they're safe/correct) without getting any value for
that. I did note there's already some inconsistency with nexttoward*,
which do it at least partly more like what you're proposing, so making
this entire family of functions consistent is a possible motivation.

> > but i'm not against the change, it is likely better on modern
> > machines. did you try to benchmark it? or check the code size?
> 
> I STILL don't run a system supported by musl.
> The code is of course smaller ... but not as small and fast as a
> proper i386 or AMD64 assembly implementation ... which I can
> post upon request.

Full asm functions are not wanted; it's something we're trying to get
rid of in favor of just using very small/single-insn asm statements
with proper constraints, where it's sufficiently beneficial to have
asm at all. But I'm not even clear how you could make this function
more efficient with asm. The overall logic would be exactly the same
as the C. Maybe on x86_64 there'd be some SSE instructions to let you
elide a few things?

Rich