fix(cfloat): frexp returns a [0.5,1) fraction (std::frexp semantics)#1029
Merged
Conversation
cfloat's frexp returned a fraction in [1,2) with *exp = scale() (floor(log2|x|)), which does not match std::frexp (fraction in [0.5,1), *exp = floor(log2|x|)+1) -- issue #1027. Generic code written to the std contract (and every other Universal type's frexp: dd, qd, ereal, bfloat16 all use [0.5,1)) saw the wrong exponent. Now: place the fraction at scale -1 (*exp = scale()+1). Extreme low-range configs (es <= 2, minimum normal exponent >= 0) cannot represent any value below 1.0 as a normal, so [0.5,1) is unachievable there; those fall back to the [1,2) fraction. ldexp is UNCHANGED -- it rebuilds the exponent from scale(), so the round-trip ldexp(frexp(x,&e),e) == x holds in every case. Also added the std special cases: +-0/inf/nan return unchanged with *exp = 0 (the old code corrupted them). Dependency sweep (the reason this is safe): the only callers of cfloat's frexp are round-trip tests, which are convention-agnostic; no generic/templated code instantiates cfloat's frexp expecting [1,2); manipulators/attributes/conversions do not use it. So the change is confined to frexp + its test. Test (fractional.cpp): assert the [0.5,1) fraction range for normal inputs where representable, plus frexp(0)/inf/nan special cases; added half and cfloat<16,8> coverage. (Note: a separate cfloat quirk -- isnormal() reports true for +-0 -- required excluding zero from the range assertion explicitly.) Verified gcc + clang: cfloat_fractional passes; the #1022 elreal oracle (which reads cfloat bits directly) is regression-clean. Resolves #1027 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR fixes cfloat's Changesfrexp Implementation and Verification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 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: 1
🤖 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.
Inline comments:
In `@static/float/cfloat/math/fractional.cpp`:
- Around line 51-58: The frexp special-case tests for TestType only check the
returned value classification but leave the exponent variable e initialized to
0, so a failure to set exp isn't detected; change the local exponent
initialization in both frexp(inf, &e) and frexp(nan, &e) blocks to a non-zero
sentinel (e.g., -1), then after calling frexp assert that e == 0 as well as
f.isinf()/f.isnan(), and on failure increment nrOfFailedTests and print a clear
message (use the existing reportTestCases flag and messages like "frexp(inf)
FAIL: exp != 0" / "frexp(nan) FAIL: exp != 0") so both classification and
exponent reset are validated for TestType.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6099b684-88d4-4d40-9514-b8c98d7fa482
📒 Files selected for processing (3)
elastic/elreal/arithmetic/exact_value_oracle.cppinclude/sw/universal/number/cfloat/cfloat_impl.hppstatic/float/cfloat/math/fractional.cpp
Comment on lines
+51
to
+58
| if (std::numeric_limits<TestType>::has_infinity) { | ||
| TestType inf; inf.setinf(false); int e = 0; TestType f = frexp(inf, &e); | ||
| if (!f.isinf()) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(inf) FAIL\n"; } | ||
| } | ||
| if (std::numeric_limits<TestType>::has_quiet_NaN) { | ||
| TestType nan; nan.setnan(); int e = 0; TestType f = frexp(nan, &e); | ||
| if (!f.isnan()) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(nan) FAIL\n"; } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate exp == 0 for inf/nan special cases, not just classification.
Line 52 and Line 56 initialize e to 0, so these checks cannot detect failure to reset exp. Seed with a non-zero sentinel and assert e == 0 after frexp.
Proposed test tightening
if (std::numeric_limits<TestType>::has_infinity) {
- TestType inf; inf.setinf(false); int e = 0; TestType f = frexp(inf, &e);
- if (!f.isinf()) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(inf) FAIL\n"; }
+ TestType inf; inf.setinf(false); int e = -99; TestType f = frexp(inf, &e);
+ if (!f.isinf() || e != 0) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(inf) FAIL: e=" << e << '\n'; }
}
if (std::numeric_limits<TestType>::has_quiet_NaN) {
- TestType nan; nan.setnan(); int e = 0; TestType f = frexp(nan, &e);
- if (!f.isnan()) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(nan) FAIL\n"; }
+ TestType nan; nan.setnan(); int e = -99; TestType f = frexp(nan, &e);
+ if (!f.isnan() || e != 0) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(nan) FAIL: e=" << e << '\n'; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (std::numeric_limits<TestType>::has_infinity) { | |
| TestType inf; inf.setinf(false); int e = 0; TestType f = frexp(inf, &e); | |
| if (!f.isinf()) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(inf) FAIL\n"; } | |
| } | |
| if (std::numeric_limits<TestType>::has_quiet_NaN) { | |
| TestType nan; nan.setnan(); int e = 0; TestType f = frexp(nan, &e); | |
| if (!f.isnan()) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(nan) FAIL\n"; } | |
| } | |
| if (std::numeric_limits<TestType>::has_infinity) { | |
| TestType inf; inf.setinf(false); int e = -99; TestType f = frexp(inf, &e); | |
| if (!f.isinf() || e != 0) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(inf) FAIL: e=" << e << '\n'; } | |
| } | |
| if (std::numeric_limits<TestType>::has_quiet_NaN) { | |
| TestType nan; nan.setnan(); int e = -99; TestType f = frexp(nan, &e); | |
| if (!f.isnan() || e != 0) { ++nrOfFailedTests; if (reportTestCases) std::cout << "frexp(nan) FAIL: e=" << e << '\n'; } | |
| } |
🤖 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/fractional.cpp` around lines 51 - 58, The frexp
special-case tests for TestType only check the returned value classification but
leave the exponent variable e initialized to 0, so a failure to set exp isn't
detected; change the local exponent initialization in both frexp(inf, &e) and
frexp(nan, &e) blocks to a non-zero sentinel (e.g., -1), then after calling
frexp assert that e == 0 as well as f.isinf()/f.isnan(), and on failure
increment nrOfFailedTests and print a clear message (use the existing
reportTestCases flag and messages like "frexp(inf) FAIL: exp != 0" / "frexp(nan)
FAIL: exp != 0") so both classification and exponent reset are validated for
TestType.
Coverage Report for CI Build 26588939352Warning 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.01%) to 84.017%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions3 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
2 tasks
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
cfloat's
frexpreturned a fraction in [1,2) with*exp = scale()(= floor(log2|x|)), which does not matchstd::frexp(fraction in [0.5,1),*exp = floor(log2|x|)+1) -- issue #1027. Generic code written to the std contract -- and every other Universal type'sfrexp(dd,qd,ereal,bfloat16all use [0.5,1)) -- saw an off-by-one exponent. cfloat was the outlier.The fix (
cfloat_impl.hpp)-1so|fraction|lands in [0.5,1);*exp = scale()+1.ldexpunchanged -- it rebuilds the exponent fromscale(), soldexp(frexp(x,&e),e) == xholds for every input (the changesetexponent(0)->(-1)is invisible to the round-trip).+-0/inf/nanreturn unchanged with*exp = 0(the old code ransetexponenton them and corrupted them).Dependency sweep (why this is safe -- the requested analysis)
Swept every
frexpcall site acrossinclude/,static/,elastic/,applications/,tools/:fractional.cpp) -- convention-agnostic (they only assertldexp(frexp(x),e)==x).dd/qd/ereal/cascade callers use their own frexp (already [0.5,1)).manipulators/attributes/conversions/numeric_limitsdo not use frexp.ldexpchange, no other types.Changes
include/sw/universal/number/cfloat/cfloat_impl.hpp-- frexp std semantics + special cases + es<=2 fallbackstatic/float/cfloat/math/fractional.cpp-- assert [0.5,1) fraction range for normals (where representable),frexp(0)/inf/nancases, +half/cfloat<16,8>coverageelastic/elreal/arithmetic/exact_value_oracle.cpp-- comment updated (cfloat frexp/floor now fixed; oracle keeps its self-contained bit-based extraction)Test Results
Note (separate, not fixed here)
cfloat's
isnormal()reports true for ±0 (std::isnormal(0)is false). It didn't affect frexp (which usesiszero()), but I had to exclude zero from the test's range assertion explicitly. Happy to file it if you'd like.Test plan
Resolves #1027
Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
frexpbehavior to correctly match standard library semantics when handling special values such as ±0, infinity, and NaN.Documentation
Tests