Page MenuHomeFreeBSD

lib/msun: Fix tgammal(3) on IEEE 128-bit platforms
ClosedPublic

Authored by markm on Mar 1 2024, 4:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 6:03 AM
Unknown Object (File)
Tue, Dec 24, 7:02 PM
Unknown Object (File)
Sun, Dec 22, 4:12 AM
Unknown Object (File)
Fri, Dec 20, 11:16 AM
Unknown Object (File)
Nov 24 2024, 7:37 PM
Unknown Object (File)
Nov 24 2024, 4:29 PM
Unknown Object (File)
Nov 24 2024, 11:59 AM
Unknown Object (File)
Nov 24 2024, 8:11 AM

Details

Summary

lib/msun: Fix tgammal(3) on IEEE 128-bit platforms

Undo the 80-bit "stub" implementation of the 128-bit long double
tgammal(3) function. The latest (as of Feb 2024) version of the
src/contrib/arm-optimised-routines library includes a standalone,
full 128-bit replacement. This needs a small bit of wrapping to
fit it in, but is otherwise a drop-in replacement.

Testing this is hard, as most maths packages blow up as soon as
their 80-bit floating-point capability is exceeded. With 128-bit
tgammal(), this is easy to do, and this is the range that needs to
be checked the most carefully. Using my copy of Maple, I was able
to check that the output was within a few ULP of the correct answer,
right up to the point of 128-bit over- and underflow. Additionally,
the results are no worse, and indeed better than the 80-bit version.

Steve Kargl sent me his libm testing code, which I used to verify
that the excpetions for certain key values were correct. Testd in
this case were +-inf, +- NaN, +-1 and +-0.

Steve's code needs GNU libraries for bignums, so can't (I presume)
be committed to the src tree.

Test Plan

I've done a fair bit of manual testing against the Gamma() function
in Maple 2021, and it seems to be OK. Graphing the output is tricky
because 1) the output gets very large very quickly (by definition),
and 2) the really interesting range exceeds the floating-point
capability of Maple, which is how Maple draws graphs. Other checks
include graphing the absolute difference, the absolute proportional
difference, and the logs of these, which are easier to see. All of
these look OK.

Steve Kargl sent me his libm testing code, which I used to verify
that the excpetions for certain key values were correct. Testd in
this case were +-inf, +- NaN, +-1 and +-0.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 56380
Build 53268: arc lint + arc unit

Event Timeline

markm requested review of this revision.Mar 1 2024, 4:13 PM
markm retitled this revision from Undo the 80-bit "stub" implementation of the 128-bit long double tgammal(3) function. The latest (as of Feb 2024) version of the src/contrib/arm-optimised-routines library includes a standalone, full 128-bit replacement. This needs a small bit of... to Undo the 80-bit "stub" implementation of the 128-bit long doubletgammal(3) function. The latest (as of Feb 2024) version of thesrc/contrib/arm-optimised-routines library includes a standalone,full 128-bit replacement. This needs a small bit of....Mar 1 2024, 4:15 PM
markm added a reviewer: theraven.
markm added inline comments.
lib/msun/ld128/b_tgammal.c
4

David - are you OK with me replacing your copyright like this?

lib/msun/ld128/b_tgammal.c
4

Very happy, please do!

markm marked an inline comment as done.Mar 1 2024, 4:56 PM

Fix minor spello in the commit message.

lib/msun/ld128/b_tgammal.c
4

Thanks!

markm edited the test plan for this revision. (Show Details)

I wonder if it would be better to include tgamma128.c from b_tgammal.c and define tgamma128 as tgammal. I've done similar things to pull in the string functions from the Arm optimised routines into libc.

lib/msun/Makefile
130

Should we be including tgamma128.c when LDBL_PREC != 113?

lib/msun/man/lgamma.3
213

I think Arm is the preferred capitalization

markm marked 2 inline comments as done.Mar 1 2024, 6:45 PM

Give this commit message a proper title

markm retitled this revision from Undo the 80-bit "stub" implementation of the 128-bit long doubletgammal(3) function. The latest (as of Feb 2024) version of thesrc/contrib/arm-optimised-routines library includes a standalone,full 128-bit replacement. This needs a small bit of... to lib/msun: Fix tgammal(3) on IEEE 128-bit platforms.Mar 2 2024, 2:30 PM

I think this is good to go, modulo the nits i pointed out, none of which are major.

lib/msun/ld128/b_tgammal.c
25

With SPDX, you don't need the boilerplate anymore. For a file this short, maybe remove it

lib/msun/man/lgamma.3
212
  1. Or maybe even 14.1

Judt saw Andy's #define comment. That would be better since it eliminates a call... if you can do that...

Also, is there a test that could be added to ensure we have the right answer at the improved precision?

lib/msun/man/lgamma.3
212

I'll update this bit as the MFCs happen.

Shorten boilerplate in response to review.

markm marked an inline comment as done.Mar 2 2024, 3:35 PM

Take Andy's very good advice and avoid the wrapper function by
#define/#include magic.

Remove comment with low signal content.

This revision is now accepted and ready to land.Mar 11 2024, 3:13 PM

Update comment concerning testing using Steve Kargl's code. This code
needs GNU libraries, so cannot be committed to the source tree.

This revision now requires review to proceed.Mar 17 2024, 2:09 PM
markm edited the test plan for this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Mar 18 2024, 9:54 AM
This revision was automatically updated to reflect the committed changes.