fix(cfloat): native trunc/floor/ceil/round (correct for >53-bit significands)#1028
Merged
Conversation
…ficands) trunc/round/floor/ceil computed `std::X(double(x))`, which is wrong for cfloats with more than 53 significand bits: any integer above 2^53 loses its low bits in the double conversion, so e.g. floor(2^60+8) did not return 2^60+8 (#1026). Now implemented directly on the cfloat encoding: - trunc: clear the fraction bits below 2^0 per the unbiased scale (scale>=fbits -> integral; subnormal or scale<0 -> signed zero). Pure bit op, no arithmetic. - floor/ceil: trunc, then -/+ 1 for the negative/positive non-integral case. - round: ties away from zero, decided from the 2^-1 fraction bit for |x|>=1 and from 2|x|>=1 for |x|<1 -- never constructing 0.5, which some low-range cfloats (e.g. cfloat<8,2>) cannot represent. Test (static/float/cfloat/math/truncate.cpp): added exhaustive trunc/round coverage (was floor/ceil only), a wide-precision verifier on cfloat<128,15> (2^60+8 +/- exact fractions, both signs, vs cfloat-constructed references -- no double), and fixed the result accumulation (`=` -> `+=`). Verified gcc + clang: exhaustive cfloat<8,2>/half/bfloat_t match the representable std reference; wide cfloat<128,15> exact; cfloat_fractional (fmod) still passes. Resolves #1026 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR fixes truncation functions (trunc, floor, ceil, round) in cfloat to operate natively on bit encoding instead of routing through Changescfloat Truncation Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
static/float/cfloat/math/truncate.cpp (2)
161-167: 💤 Low valueManual testing section doesn't test all four functions.
The
MANUAL_TESTINGblock only testsfloorandceil, while the regression block tests all four functions (floor,ceil,trunc,round). For consistency, consider addingVerifyTruncandVerifyRoundcalls here as well.📝 Suggested addition
nrOfFailedTestCases += ReportTestResult(VerifyFloor< cfloat<8, 2, uint8_t> >(reportTestCases), "floor", "cfloat<8,2>"); nrOfFailedTestCases += ReportTestResult(VerifyCeil < cfloat<8, 2, uint8_t> >(reportTestCases), "ceil ", "cfloat<8,2>"); + nrOfFailedTestCases += ReportTestResult(VerifyTrunc< cfloat<8, 2, uint8_t> >(reportTestCases), "trunc", "cfloat<8,2>"); + nrOfFailedTestCases += ReportTestResult(VerifyRound< cfloat<8, 2, uint8_t> >(reportTestCases), "round", "cfloat<8,2>");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/float/cfloat/math/truncate.cpp` around lines 161 - 167, The MANUAL_TESTING block only calls VerifyFloor and VerifyCeil; add calls to VerifyTrunc< cfloat<8, 2, uint8_t> > and VerifyRound< cfloat<8, 2, uint8_t> > and accumulate their results into nrOfFailedTestCases using ReportTestResult, mirroring the existing ReportTestResult pattern for "trunc" and "round" labels; keep the ReportTestSuiteResults(test_suite, nrOfFailedTestCases) and return EXIT_SUCCESS unchanged.
12-16: 💤 Low valueComment mentions 53-bit significands but code uses
float(24-bit precision).The comment states these verifiers are valid for types with "≤ 53 significand bits," but the reference computation uses
float(a)which only has ~24 bits of mantissa precision. This is correct for the current test types (all have < 24 significand bits), but the comment is misleading if someone adds a test type with 25-53 bits.Consider either updating the comment to reflect the actual
floatlimitation, or usingdoublefor the reference to match the comment's claim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/float/cfloat/math/truncate.cpp` around lines 12 - 16, The comment claims the verifier uses an exact reference for types with "≤ 53 significand bits" but the code uses float(a) (≈24-bit mantissa); either correct the comment or use double for the reference. Fix by replacing float(a) with double(a) in the small-cfloat exhaustive verifier where native trunc/floor/ceil/round are compared, and update the nearby comment to state that double is used and is exact for ≤53-bit significands (leave VerifyWideTruncation as-is for wider cases). Ensure you modify the same verifier function/block that currently calls float(a) and adjust the comment text to reflect the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@static/float/cfloat/math/truncate.cpp`:
- Around line 161-167: The MANUAL_TESTING block only calls VerifyFloor and
VerifyCeil; add calls to VerifyTrunc< cfloat<8, 2, uint8_t> > and VerifyRound<
cfloat<8, 2, uint8_t> > and accumulate their results into nrOfFailedTestCases
using ReportTestResult, mirroring the existing ReportTestResult pattern for
"trunc" and "round" labels; keep the ReportTestSuiteResults(test_suite,
nrOfFailedTestCases) and return EXIT_SUCCESS unchanged.
- Around line 12-16: The comment claims the verifier uses an exact reference for
types with "≤ 53 significand bits" but the code uses float(a) (≈24-bit
mantissa); either correct the comment or use double for the reference. Fix by
replacing float(a) with double(a) in the small-cfloat exhaustive verifier where
native trunc/floor/ceil/round are compared, and update the nearby comment to
state that double is used and is exact for ≤53-bit significands (leave
VerifyWideTruncation as-is for wider cases). Ensure you modify the same verifier
function/block that currently calls float(a) and adjust the comment text to
reflect the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 009d4528-26d7-4af6-849b-5af460868471
📒 Files selected for processing (2)
include/sw/universal/number/cfloat/math/functions/truncate.hppstatic/float/cfloat/math/truncate.cpp
Coverage Report for CI Build 26585831192Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.008%) to 84.007%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions10 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
trunc/round/floor/ceilforcfloat<>computedstd::X(double(x)). That is wrong for cfloats with more than 53 significand bits: any integer above 2^53 loses its low bits in thedoubleconversion, so e.g.floor(2^60 + 8)did not return2^60 + 8(#1026). Reimplemented directly on the cfloat encoding.The fix (
truncate.hpp)scale(scale >= fbits→ integral; subnormal orscale < 0→ signed zero). Pure bit operation.trunc, then-/+ 1for the negative/positive non-integral case.|x| >= 1and from2|x| >= 1for|x| < 1. Deliberately never constructs0.5, which some low-range cfloats (e.g.cfloat<8,2>) cannot represent -- an early version did and regressed those.All derived ops use cfloat's own (correctly rounded) arithmetic/comparisons; no
doubleround-trip.Test (
static/float/cfloat/math/truncate.cpp)cfloat<128,15>:2^60+8± exact fractions, both signs, checked against cfloat-constructed references (nodouble).=→+=, a latent bug that hid all but the last sub-test).Test Results
Exhaustive
cfloat<8,2>/half/bfloat_tmatch the representable std reference; widecfloat<128,15>exact.Notes
frexpreturns [1,2) vs std's [0.5,1)) is separate.Test plan
Resolves #1026
Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests