Repository navigation
fix(libc): compute acosl with FreeBSD msun's ld80 code as the bundled libc.prx does - #11260
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact head 84591b8434421fdc06a556f585313dc5bd2da4e2. The acosl implementation follows the pinned FreeBSD ld80 algorithm and its tested edge handling; the PR’s reported native comparison covers 20 million random inputs plus boundary cases. I independently checked the diff and conventions in the PR worktree earlier in this sweep; both passed. All hosted eligibility, conventions, intake, report, relinker, register-context and write-watch jobs are now green. Linux and Windows product jobs were skipped, and I did not independently run the native differential here.
I found no blocking issue in the implementation or evidence for this change, so I approve this head.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
I found an x87 precision-state gap while reviewing the follow-on atanl/exp2l ports. This acosl implementation also needs 64-bit x87 precision according to its own debt entry, but it does not scope or restore the x87 control word, and its tests do not run with the 53-bit control setting documented for guest threads (0x27f). FreeBSD's referenced ld80 routine uses ENTERI()/RETURNI() for the computation for this reason; the random comparison under the harness's default environment does not cover callers that arrive with a different precision mode. Please add a scoped precision-control path and a regression that checks the result under 53-bit precision and verifies the original control word is restored, or provide console evidence that the caller's precision mode is intentionally preserved and test both modes. I am updating my earlier approval because this correctness gap affects the stated bit-for-bit behavior. Hosted builds remain skipped for this libc-only path; the comparison and focused test results in the PR are author-reported, not independently rerun here.
|
Windows run of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
84591b8 to
ff13b2e
Compare
|
@MarcZX10 The console keeps the caller's precision, so I took the second option (evidence and tests at both precisions) rather than forcing 64 bits:
Forcing 64 bits would make the export differ from the binary whenever the caller runs at 53 bits. Under AnyPS5, the 53-bit case comes from guest threads not starting with FreeBSD's Changes in ff13b2e:
|
|
Windows run of this head
AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
The FreeBSD msun port is backed by detailed bitwise comparisons against the bundled libc.prx, across inputs, precisions, rounding modes, errno, and exception flags. The committed known-answer cases also cover the important subnormal and non-canonical paths.
I cannot approve this submitted head against current main. Its merge-base is 68cc9d4, but current main is 3fea2a9. Since that base, main has changed all three paths this PR edits: Math.cpp, GuestMath.cpp, and docs/dev/debt/libc.md. The Tested section names a run on main 5b5d24c, which is not the base of the submitted head, so the report does not establish that this exact diff passed on the integrated tree.
Please rebase the #11077-dependent branch onto current main, resolve the math/debt integration, and rerun guest_math plus the reference comparison on the resulting head. I found no specific arithmetic defect in the inspected cbrtl-adjacent stack code; this request is for current-tree correctness and matching test evidence before approval.
|
Correction to the last sentence of my review: #11260 changes |
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the other open libc PRs #11262, #11266, #11632, #11634, #11278, #11270, #11283, #10929, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
ff13b2e to
23a6c80
Compare
|
@MarcZX10 Rebased onto
The new rounding-mode runs found one difference, now fixed. GCC loaded the
|
|
@AlexFigas a survey that may help plan the rest of the ld80 family. I swept every one-argument
Exact on Since you're porting this family, I don't want to step on your plans. Which of the uncovered ones are you taking? I could take a separate group, for example AI-assisted: yes (Claude Code). |
|
@AlexFigas the two-argument half of the same sweep (PPSA07923, 200,000 random pairs per function):
Exact on AI-assisted: yes (Claude Code). |
|
Retracted, see the correction below: the standalone build used here does not reproduce FreeBSD results faithfully. @AlexFigas one more data point for the planning question, on the
Every difference I looked at is 1 ulp, e.g. AI-assisted: yes (Claude Code). |
|
@AlexFigas correction to my AI-assisted: yes (Claude Code). |
|
@AlexFigas follow-up on my correction, in case it saves you time on the remaining ld80 files. I found the main reason my standalone FreeBSD build disagreed with the titles. FreeBSD's amd64 With
AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact current head 8365772e47fbce5df3fda265bd7b194ea2a4e98d. The post-approval delta adds the required e_acosl.c copyright/license notice immediately above the port; it matches FreeBSD release/11.4.0. The reviewed acosl implementation, tests and declared dependency on #11077 remain unchanged. Hosted Ubuntu, Windows, macOS, ASan, register-context and write-watch checks pass; Linux product is skipped by workflow filters. The author reports guest_math and title-binary comparisons on the new heads. No PS5 title run is claimed.
|
@AlexFigas this settles the open
So they're Dinkumware wrappers over Would you like to add that to #11270, or shall I open it as a small follow-up stacked on #11270 (Depends on: #11270) and run the native comparison on it? Either is fine; I won't start it until you say. AI-assisted: yes (Claude Code). |
|
@AlexFigas the same check for the trig and hyperbolic group, in case it helps plan (PPSA07923, 100,000 random 80-bit arguments each). The titles' public functions are exact wrappers over their exported Dinkumware internals:
So, like Same offer as before: tell me which of these you'd rather I leave to you. AI-assisted: yes (Claude Code). |
|
@AlexFigas one more data point, for prioritising rather than correctness. In the 22 titles of my import audit (converted registries of titles on the share), none imports any of these 25
The same search finds the 8 importers of AI-assisted: yes (Claude Code). |
|
@MarcZX10 Head 84502b8 adds one commit to the approved 8365772 and merges |
|
@MarcZX10 Head c6faef7 merges current |
|
@MarcZX10 Head 39fb738 merges current |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (15, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 765 tests)Skipped (61)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@MarcZX10 Head 0de3168 merges current |
|
Checked the |
What
acoslcalled the hoststd::acos. Over 20 million random arguments in (-1, 1), the host result differed from the console's on 1,603,469 of them. This PR replaces it with FreeBSD msun'se_acosl.c(ld80), which is theacoslof thelibc.prxthat PPSA21564 bundles (0x3e5a0, the library's own code). That binary followse_acosl.cbranch for branch:0orpi + 2*pio2_lo. For anything else, including the infinities and NaN, it calls_Feraise(1), then returns(x - x) / (x - x)._Feraise(0x11b00) raisesFE_INVALIDand setserrnotoEDOM(0x21).pio2_hi + pio2_lo. Otherwise it returnspio2_hi - (x - (pio2_lo - x*r)), withr = P(x²)/Q(x²).z = (1 + x) * 0.5,s = fsqrt(z), and the result ispi - 2*(s + (r*s - pio2_lo)).df=swhose low 32 mantissa bits are cleared,c = (z - df²)/(s + df), and the result is2*(df + (r*s + c)).P,Q,pio2_hiandpio2_loare the ld80invtrig.cconstants.asinlalready reproduces them inMath.cpp(AsinNumerator,AsinDenominator,HalfPiHigh,HalfPiLow), and the binary holds the same 80-bit values.piis thee_acosl.cconstant (0xc90fdaa22168c235:4000in the binary), and the port reads it from memory as the binary does: as a compile-time constant, GCC loads it withfldpi, which rounds by the current mode, so under round-downward or toward-zero at 64 bits the results below -0.5 andacosl(-1)could come out one bit low (51,708 of 5,000,000 arguments). The port reuses those helpers and addsPi.x87 precision. The binary's
acoslhas no instruction that reads or sets the x87 control word (nofnstcw,fldcw,fldenvorfninit), so it computes at the caller's precision and leaves the control word as it found it. That is the FreeBSD source on amd64:e_acosl.cdoes not useENTERI()/RETURNI(), and those are empty outside i386 anyway (math_private.h). The port does the same. One difference had to go:pio2_hi + pio2_loandpi + 2*pio2_loare added at run time in the binary, becausepio2_hiandpio2_loareinvtrig.cexterns, so at 53 bits they round to 53 bits; GCC folded them at 64 bits. The port now adds them throughSumAtCallerPrecision, which reads the first half through avolatile. Everything else already ran at run time.The
acosldebt entry keeps what is still open: it runs at the caller's precision, so on a guest thread that starts at 53 bits (the thread-start gap theasinlentry records) it returns the binary's 53-bit result, not the console's 64-bit one; other SDK versions are not compared. The next entry no longer says "asacoslis".Licence.
e_acosl.c's copyright notice is copied verbatim directly aboveacosl, asMath.cppalready does for the msun code ofsinh. Like that copy, it keeps the source's trailing space after "this notice", the only linegit diff --checkreports.Tested
On Ubuntu 24.04 under WSL2 (GCC 13.3, Release), head 0de3168, which merges
main813f46c into the previous head 39fb738 (maina4eca2a). The merge has one conflict, inMath.cpp: #12366 rewrotefdimandfdimfright above the hostacoslline, so the resolution keepsmain'sfdimandfdimf, this PR's notice andacosl, and drops the hostacosl. 39fb738 resolved a debt-file conflict with #11077'sfrexpentry; 84502b8 mergedmaincc26ab4 after a commit that drops the trailing space from thee_acosl.cnotice, sogit diff --checkpasses; 8365772 was rebased ontomain2227f13:libc.prx. Against the binary, 5,000,000 arguments of every kind (|x| > 1, NaNs and non-canonical encodings included) at each of 64, 53 and 24 bits, under each rounding mode, give the same result, NaN bits,errnoand flags, and the control word is unchanged after every call (60,000,000 in all).libc.prxinto an executable buffer and called itsacosl(0x3e5a0) directly; arguments below 1 in magnitude never reach an import. Against this port it gives the same 80 bits for 20,000,000 random arguments plus the exponent edges (0x3fbc–0x3fbf, 0x3ffd, 0x3ffe, ±0, ±1), with 0 mismatches, at each of 64-, 53- and 24-bit precision (control words 0x37f, 0x27f and 0x07f, set before both are called). The hostacoslmismatches on 1,603,469 of the same arguments at 64 bits. BeforeSumAtCallerPrecision, the 53-bit run mismatched on 252,624 of 2,000,000, all from those two returns.guest_mathpasses (3 runs). It gainsCheckAcosl, with 22 known-answer pairs produced by that binary. Four of them were picked because the hostacoslgets them wrong, for example0xc1d51c773e6f53e1:3ffd → 0x975d54d5f258ea8a:3fffwhere the host gives…89. It also checks that ±1.5, ±(1 + ulp), the maximum, ±∞ and NaN return NaN withEDOMandFE_INVALID. Six arguments, one or two per branch, also run at 53 bits throughExtendedBitsAt53Bits, which sets the precision field to 53 bits, calls the export, reads the control word back and restores the caller's; each must give the binary's 53-bit result and leave the control word unchanged. With the sums folded again, it aborts at0x8000000000000000:3fbd. The existingacoslchecks inCheckLongDoublestill pass. It now also requires the binary's results for three negative arguments, -1 among them, under round-downward.python3 tools/check_conventions.py --base origin/mainpasses, andgit diff --checkis clean.python3 tools/pr_overlap.py --branch fix/libc-acosl-msun-ld80reports fix(libc): compute atanl with FreeBSD msun's ld80 code as the bundled libc.prx does #11262 and fix(libc): compute exp2l with FreeBSD msun's ld80 code as the bundled libc.prx does #11266, which are stacked on this PR, and oberonlift's feat(libc): keep the guest's x87 long double exactly in lifted builds #15802, feat(libkernel): run threads for a lifted guest #15803, feat: find a lifted program's guest images, load its modules and switch its fibers #15804 and feat(ps5lift): run lifted programs and titles on AnyPS5's libraries #15807:frexpis onmainaboveacosl, and so arefdimandfdimf, which the merge kept.GuestLongDoubletype, including the hostacoslline this PR replaces, so each of feat(libc): keep the guest's x87 long double exactly in lifted builds #15802, feat(libkernel): run threads for a lifted guest #15803, feat: find a lifted program's guest images, load its modules and switch its fibers #15804 and feat(ps5lift): run lifted programs and titles on AnyPS5's libraries #15807 conflicts with this head inMath.cppandlibc.mdwhere it does not withmain(all four already conflict withmaininlibSceJobManager/Export.cpp). This PR was opened and approved first, so I have not added a dependency; the order is for that stack's author to agree.main, give the samegit merge-treeconflicts with this branch as withmain.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)