Repository navigation
fix(libc): raise inexact for nearbyintf - #12178
Conversation
|
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.
|
|
@Henryisdjfunkyho thanks, the change follows the console measurement in the existing |
|
Linux result with a control (the description has Windows only). Linux matters here because Setup: Ubuntu 22.04 under WSL2, GCC 11.4.0, glibc 2.35, Ninja Release, Tests:
Not tested: AI-assisted (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the implementation and the documentation-only follow-up. The code change uses rintf only for nearbyintf, preserving the existing NaN path, and the added mode-mismatch assertions check both MXCSR-selected results and FE_INEXACT. The debt entry accurately limits the change to the measured float behavior. I independently ran C++20 syntax-only compilation for Math.cpp and GuestMath.cpp, the conventions checker, git diff --check, and a clean merge preview with current main; hosted policy, relinker, specialized checks and Windows build pass. The reported focused runtime tests were run on base cccdaa2734, though.
Before approval, please rebase onto current main (09d952f0eb3c68aa3d36713020d60b13644fad02) and rerun the focused four-test selection on the rebased tree. This branch is 31 commits behind current main; CONTRIBUTING.md requires branches to be based on current main. I found no implementation defect in the reviewed diff. Linux full build is skipped by workflow selection, and no console remeasurement is claimed.
4c09c19 to
ff6803b
Compare
|
A comparison against titles' own Setup: I built this head
The inexact flag is fixed: every round-to-nearest and toward-zero Cause: A data point for the debt entry: the titles' AI-assisted: yes (Claude Code). |
|
The new bundled-libc comparison provides a concrete correctness blocker on the current implementation: in |
MarcZX10
left a comment
There was a problem hiding this comment.
Blocking correctness issue: the current std::rintf call in Math.cpp is compiled without -frounding-math; the exact-head bundled-libc comparison reports wrong results under FE_DOWNWARD/FE_UPWARD, despite fixing FE_INEXACT. Please use an implementation compiled with rounding-mode semantics and add tests for directed modes, signed zero and FE_INEXACT. The tested head is also 403 commits behind live main (3a40945e62), so rebase and rerun focused tests after the fix. Detailed evidence and examples are in my latest comment.
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the change. Blocking issue, confirmed against the tree and the comparison by @JJenkx above:
core/libs/prx/libc/src/Math.cpp:476:std::rintfhere is compiled without-frounding-math(core/libs/prx/libc/CMakeLists.txt:118-120sets it only forMathExact.cpp), so GCC folds it into a round-to-nearest-only sequence. TheFE_INEXACTflag is fixed, but the values are now wrong in the directed modes, wheremainwas right: e.g.FE_DOWNWARD nearbyintf(-0.5f)returns -0 (titlelibc.prx: -1),FE_UPWARD nearbyintf(-2.5f)returns -3 (title: -2), andnearbyintf(+0)/nearbyintf(0.5f)underFE_DOWNWARDreturn -0 instead of +0.
Fix: implement it in a translation unit compiled with-frounding-math(next torintf_nid_postfixinMathExact.cpp, or return that function's result), keeping the NaN path.core/libs/tests/GuestMath.cpp:128-143: the new test only covers x87 upward / MXCSR nearest and the reverse. Please add value plusFE_INEXACTchecks for all four MXCSR modes with halves of both signs and ±0, comparing result bits so -0 and +0 are distinguished. It should fail with the currentMath.cppchange.- The head is far behind
main; please rebase and rerun the focused math tests after the fix. docs/dev/debt/libc.md: the titlelibc.prxnearbyint(double) also raisesFE_INEXACTfor inexact inputs (same values asmain, per the comparison above); say so in thenearbyint and nearbyintlentry, notingnearbyintlwas not compared.
ff6803b to
b41c727
Compare
|
A native comparison of this head (
So the titles' AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The directed-rounding implementation and regression coverage address the earlier code findings, but the tested head is not integrated with the current main changes to the same four files.
What I ran
Reviewed PR head b41c727b01e69f5317294c5084becff862026488, whose merge base with current main is 17f8dfad513b134a4996991109fc9db47a3bb905. Preview-merged it onto current main 3a2b91981651569163768854a1fcdc7193b6667b; the merge is clean, but all four PR-touched paths also changed on current main. Environment: Linux x86-64, Git and GitHub CLI. I did not build or run tests locally.
git fetch https://github.com/boykopovar/AnyPS5.git main:refs/review/main-20261029 refs/pull/12178/head:refs/review/pr-12178-20261029
git merge-base refs/review/main-20261029 refs/review/pr-12178-20261029
git merge-tree --write-tree refs/review/main-20261029 refs/review/pr-12178-20261029
gh api repos/boykopovar/AnyPS5/compare/17f8dfad513b134a4996991109fc9db47a3bb905...3a2b91981651569163768854a1fcdc7193b6667b --jq '.files[].filename'
gh api repos/boykopovar/AnyPS5/pulls/12178/files --paginate --jq '.[].filename'Results
| Check | Result |
|---|---|
| Current-main merge preview | Pass; clean |
| Touched paths changed on both sides | Math.cpp, MathExact.cpp, GuestMath.cpp, and docs/dev/debt/libc.md |
| Focused tests | Author reports guest_math and guest_math_exact pass on the PR head merged onto main 319621ba2e; not rerun here |
| Local build/tests | Not run |
Command output
merge-base: 17f8dfad513b134a4996991109fc9db47a3bb905
git merge-tree --write-tree: 889ead4182337af56f6ae806a10d602e87978d5e
exit=0
changed-path intersection:
core/libs/prx/libc/src/Math.cpp
core/libs/prx/libc/src/MathExact.cpp
core/libs/tests/GuestMath.cpp
docs/dev/debt/libc.md
PR head: b41c727b01e69f5317294c5084becff862026488
current main: 3a2b91981651569163768854a1fcdc7193b6667b
Negative control
| Assertion | Change to code under test | Outcome |
|---|---|---|
| Directed-rounding regression fails without the fix | None performed by me | Not independently exercised |
Findings
Blocking
- The PR is still based on
17f8dfad513b; currentmainis3a2b91981651, and currentmainchanged each of the four files this PR edits. The preview merge is clean, but the old-base test report does not validate the integrated current-main tree. The contribution rules require keeping the branch current.
Non-blocking
- The current diff moves
nearbyintftoMathExact.cppand adds bitwise result, signed-zero, four-MXCSR-mode andFE_INEXACTassertions. That addresses the earlier directed-rounding/test-coverage findings; I found no additional code issue in this re-check.
Sources
- Current contribution rules
- PR implementation at the reviewed head
- PR regression tests at the reviewed head
Not verified
- I did not independently build or execute the math tests, test another OS, or run a console comparison. The reported Windows comparison and test results are author-provided.
To get this to a good state
Rebase onto current main and rerun guest_math and guest_math_exact on the new head. Record the exact head and merged base in the Tested section; I will re-check the rebased diff and rerun the focused validation available here.
b41c727 to
472500c
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The author rebased onto current main, reran the four focused math tests on the exact head, and the directed-rounding fix and regression cases match the reported behavior; I found no blocking issue.
What I ran
- Reviewed exact PR head
472500cb80807684b624df36c2792cf9d2bf9946against currentmain52c09d567d2fa453494dd720fcf53a98149468e4. - Ran
python3 tools/check_conventions.py --base refs/review/main-20261029bandgit diff --check refs/review/main-20261029b...refs/review/pr-12178-20261029blocally. - Inspected the implementation, four-mode positive/negative half-way cases, signed-zero checks, exception-flag assertions, debt text, latest hosted checks, and the author's exact-head test report.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree 582cf9ca2652b6b1e87b3f910510f8f5458f713d |
| Conventions | Passed locally |
| Diff whitespace | Passed locally |
| Focused tests on exact head | Author reports guest_math, guest_math_exact, guest_floating_point, and guest_libc_static_imports: 4/4 passed on Windows x86-64 / WinLibs GCC 15.2 Release |
| Hosted checks | Windows and Ubuntu relinker/ASan and registration-context checks passed; Linux product job skipped; macOS and write-watch jobs were pending when checked |
Command output
$ python3 tools/check_conventions.py --base refs/review/main-20261029b
All conventions checks passed.
CONVENTIONS_EXIT:0
$ git diff --check refs/review/main-20261029b...refs/review/pr-12178-20261029b
DIFF_CHECK_EXIT:0
$ git merge-tree --write-tree refs/review/main-20261029b refs/review/pr-12178-20261029b
git merge-tree --write-tree: 582cf9ca2652b6b1e87b3f910510f8f5458f713d
exit=0
The focused-test result above is from the author's report in the PR description, not a local test run by me.
Negative control
- Not rerun on this head. The PR description records a prior negative control: moving the implementation back to
Math.cppwhile retaining the new tests madeguest_mathfail; restoring the fix passed.
Findings
Blocking
- None found.
Non-blocking
- The exact-input console flag behavior and Linux runtime behavior remain unmeasured; the description identifies these limits. The current diff is limited to
nearbyintf, its focused tests, and the corresponding debt entry.
Sources
- Implementation at reviewed head
- Regression tests at reviewed head
- Debt entry at reviewed head
- Contribution rules at current
main
Not verified
- I did not independently run the focused tests, execute a title or console comparison, run Linux product tests, or verify the final result on macOS/write-watch jobs that were still queued.
|
Independent Linux CPU qualification of exact head
**Remaining raw-hardware flag caveat:**384 subnormal cases with DAZ off set MXCSR bit1 (denormal operand) in the candidate, absent in both references. This bit is outside This is bundled-library execution on a Linux host, not a fresh console measurement. Unmasked traps, other SDK variants, Windows at this head, GPU/title execution and performance are outside this run. Probe source, exact pins, source delta, full build/tests and all comparison rows, SHA-256 AI-assisted: yes (Codex). |
What
Fix
nearbyintfto use MXCSR-controlled rounding and raiseFE_INEXACTfor measured inexact inputs. Move itsrintfimplementation toMathExact.cpp, which is compiled with rounding-math semantics, so directed rounding modes and signed zero work correctly. Preserve the NaN path. Tests cover all four MXCSR modes, positive and negative halves, signed zero, and inexact flags. The debt note records the title-libc comparison fornearbyintand the unmeasurednearbyintlbehavior.Tested
Rebased head
472500cb80807684b624df36c2792cf9d2bf9946onto main52c09d567d2fa453494dd720fcf53a98149468e4. Windows x86-64, WinLibs GCC 15.2.0, Ninja Release: rebuilt the four focused targets and ranctest --test-dir build --output-on-failure -R '^(guest_math|guest_math_exact|guest_floating_point|guest_libc_static_imports)$': 4/4 passed, including both tests requested in the latest review.python tools/check_conventions.py --base origin/mainandgit diff --check origin/main...HEADpassed. Independent review found no actionable rebase issue; it did not run tests. The full build and full-suite results below are from earlier heads, not this rebase.Earlier validation:
Windows x86-64, WinLibs GCC 15.2.0, Ninja Release,
BUILD_TESTING=ON. On headb41c727b01e69f5317294c5084becff862026488, based on main17f8dfad513b134a4996991109fc9db47a3bb905:cmake --build build --parallel 8 cmake --build build --target guest_math_tests guest_math_exact_tests guest_floating_point_tests guest_libc_static_imports_tests --parallel 8 ctest --test-dir build --output-on-failure -R '^(guest_math|guest_math_exact|guest_floating_point|guest_libc_static_imports)$' python tools/check_conventions.py --base origin/main git diff --check origin/main...HEADFull build succeeded; the four requested focused tests passed 4/4 and conventions and diff checks passed. On the preceding rebased head, moving the implementation back to
Math.cppwhile retaining the new tests madeguest_mathfail; restoring the fix passed. An independent Luna high review found no actionable issue; exact-input console flag behavior remains unmeasured.On the preceding base
04fefebe90,ctest --test-dir build --output-on-failure --parallel 8 --timeout 120reported 18 failures out of 2,837 tests, with skips. Failures were in two AGC tests and 16 VideoOut tests. On the final base, three representative failures (agc_driver_recorder,agc_driver_sample_order_grid_reference,video_out_flip_labels) also failed after replacing both changed math source files with their main versions. VideoOut reportedvkGetPhysicalDeviceSurfaceCapabilitiesKHR: Vulkan result -13. This control does not establish the cause of every failure. The full suite was not rerun on the final base.The earlier overlap scan found only this PR's prior head as a confirmed conflict. Some stale PRs could not be compared automatically; their changed lines were inspected for nearbyint-related changes. No Linux runtime test, new console measurement, title execution, or performance result is claimed for this head. Hosted checks must rerun after the update.
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)