feat(elreal): Phase 1 -- block<FpType> representation#934
Merged
Conversation
Phase 1 of the McCleeary LFPERA reimplementation (epic #923). Lands the foundational `block<FpType>` type with the int32_t `exp_offset` escape multiplier, the 0-overlap predicate, pretty-printers, and a sweep test suite parameterised over float, double, half, bfloat16, cfloat<24,5>, and cfloat<32,8>. What `block<FpType>` holds - `FpType v`: sign + biased exponent + k-bit significand packed into the host FP type. - `int32_t exp_offset`: multiplier in units of `2^E_FpType` so a single block can carry an exponent far outside the host type's hardware range, which Phase 7 transcendentals will need. Effective value `v * 2^(exp_offset * 2^E_FpType)`. Combined exponent `E(b) = scale_of(v) + exp_offset * 2^E_FpType`. McCleeary's k = numeric_limits<FpType>::digits. Files - `include/sw/universal/number/elreal/exp_field_width.hpp`: trait that yields E for float (8), double (11), bfloat16 (8), cfloat<N,E> (E). - `include/sw/universal/number/elreal/block.hpp`: block<FpType> struct plus `zero_overlap(b1, b2)` free function. Trivial layout (no in-class initialisers), branches on `has_universal_fp_api_v<FpType>` to pick between Universal's .sign()/.scale() and native std::signbit/std::ilogb. - `include/sw/universal/number/elreal/block_manipulators.hpp`: to_binary, to_hex, color_print. to_hex memcpys the bit pattern for native float and double; for Universal wrappers it falls back to the human-readable form (to avoid cross-header churn -- richer rendering can come later). - `include/sw/universal/number/elreal/elreal.hpp`: umbrella header. - `include/sw/universal/number/elreal/elreal_fwd.hpp`: forward decl. Tests under `elastic/elreal/block/` - construction.cpp: trivial-copyable + trivial-destructible assertions; value and value+offset construction; copy semantics; compile-time constants (k, E, exp_step). - accessors.cpp: sign() on +/-1.5; scale_of_v() on 1, 2, 0.5; exponent() with offset; value_as<double>() roundtrip. - predicates.cpp: is_zero_block on 0, on 0-with-offset, on nonzero; is_normalised on normal and zero. - zero_overlap.cpp: boundary at E(b2)=-k; in-the-gap violation at -k+1; wide gap; zero blocks trivially 0-overlap; cross-offset gap; reversed pair fails. - exp_offset.cpp: offset 0, +/-1, +100, INT32_MAX; verifies int64 arithmetic does not overflow. - printers.cpp: smoke tests; non-empty output for every supported FpType. Build wiring - New CMake option `UNIVERSAL_BUILD_NUMBER_ELREALS` (mirrors EREALS). - Cascade into the ELASTICS umbrella. - Cascade into CI_LITE so portability is validated on every PR. - `elastic/elreal/CMakeLists.txt` registers the block tests via compile_all under `el_block_*`. Validation - gcc 13.3: all 6 tests PASS. - clang 18.1: all 6 tests PASS. - k values: float=24, double=53, half=11, bfloat16=7 (Universal numeric_limits quirk: dissertation k would be 8), cfloat<24,5>=19, cfloat<32,8>=24. Out of scope for Phase 1 - ZBCL co-list construction (Phase 2, #926) - twoSum / twoMult / twoDiv on blocks (Phase 3, #927) - Stream-level arithmetic (Phase 4+) Resolves #925 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhase‑1 elreal: adds exponent-field traits, core Changeselreal Phase 1: block Foundation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 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: 3
🧹 Nitpick comments (2)
include/sw/universal/number/elreal/block_manipulators.hpp (1)
40-42: ⚡ Quick winPrefer
std::fabsfor clarity when computing absolute value.Line 41 manually negates
abs_fracif it's negative. While this works, usingstd::fabs(abs_frac)orabs_frac = std::abs(abs_frac)is more idiomatic and clearer.♻️ Proposed refactor for clarity
double abs_frac = static_cast<double>(b.v) / std::ldexp(1.0, b.scale_of_v()); - if (abs_frac < 0) abs_frac = -abs_frac; + abs_frac = std::fabs(abs_frac); s << " * " << std::scientific << std::setprecision(6) << abs_frac;🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp` around lines 40 - 42, The code computes abs_frac then manually negates it when negative; replace the manual sign check with a call to the standard absolute function (e.g., use std::fabs(abs_frac) or std::abs(abs_frac)) so the assignment to abs_frac uses the standard library for clarity and intent; update the expression around the variable abs_frac in the block that constructs the scientific string (the lines around abs_frac = static_cast<double>(b.v) / std::ldexp(1.0, b.scale_of_v()) and the subsequent sign handling) to use std::fabs/ std::abs instead of the if (abs_frac < 0) abs_frac = -abs_frac; pattern.elastic/elreal/block/predicates.cpp (1)
36-42: ⚡ Quick winAdd boundary checks for is_normalised() contract
Please add explicit negative cases for values below and above the normalized window (for example
0.75and2.0) so the test enforces the[1,2)invariant.Proposed test additions
// is_normalised on normal values B norm{ FpType{1.5}, 0 }; if (!norm.is_normalised()) { std::cout << tag << " 1.5 not normalised\n"; ++nrFailures; } + // boundary/out-of-range checks for [1,2) + B below{ FpType{0.75}, 0 }; + B above{ FpType{2.0}, 0 }; + if (below.is_normalised()) { std::cout << tag << " 0.75 incorrectly normalised\n"; ++nrFailures; } + if (above.is_normalised()) { std::cout << tag << " 2.0 incorrectly normalised\n"; ++nrFailures; } + // is_normalised on zero -> false if (zero.is_normalised()) { std::cout << tag << " zero claimed normalised\n"; ++nrFailures; }🤖 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 `@elastic/elreal/block/predicates.cpp` around lines 36 - 42, Add explicit negative test cases to enforce the [1,2) invariant by checking is_normalised() returns false for values just below and just above the normalized window: construct B test instances like B low{ FpType{0.75}, 0 } and B high{ FpType{2.0}, 0 } (matching the existing pattern using FpType and B), then assert !low.is_normalised() and !high.is_normalised(), incrementing nrFailures and printing the same tag-style message on failure; place these checks alongside the existing norm and zero checks so the contract for is_normalised() is fully validated.
🤖 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 `@elastic/elreal/block/accessors.cpp`:
- Around line 49-60: The floating comparison in the test using B (constructed
from FpType{3.25}) reads recovered = v.template value_as<double>() and compares
std::abs(recovered - host_stored) > 0.0 which is effectively exact-equality;
update the condition in the check that reports failures (the if that currently
uses std::abs(... ) > 0.0) to either perform a bit-pattern mismatch check using
recovered != host_stored (to catch true unequal doubles) or use a small epsilon
(e.g., compare std::abs(recovered - host_stored) > epsilon) appropriate for
FpType/host_stored to allow rounding differences, keeping the rest of the
failure reporting (tag, nrFailures increment) unchanged.
In `@include/sw/universal/number/elreal/block.hpp`:
- Around line 110-126: The is_normalised() logic must enforce the documented
invariant that |v| is in [1,2); update both branches to test the absolute value
range instead of (or in addition to) fpclassify/scale: keep the early
is_zero_block() check, compute abs_v from v (using v.sign() ? -v : v) and return
(abs_v >= FpType{1} && abs_v < FpType{2}) for the universal FpType path (and do
the same for the native path instead of relying only on std::fpclassify(v) ==
FP_NORMAL); this ensures is_normalised() only accepts values in [1,2) and
rejects 0, subnormals and values like 0.75 or 3.0.
In `@include/sw/universal/number/elreal/exp_field_width.hpp`:
- Around line 42-44: exp_step_v currently does an unchecked left shift which is
undefined for exp_field_width_v<T> >= 63; change its definition to guard the
shift: compute the step as (exp_field_width_v<T> < 63 ?
static_cast<std::int64_t>(1) << exp_field_width_v<T> :
std::numeric_limits<std::int64_t>::max()) and include <limits> if needed, so
exp_step_v<T> never performs an invalid shift; reference the template variable
exp_step_v and exp_field_width_v<T> when making this change.
---
Nitpick comments:
In `@elastic/elreal/block/predicates.cpp`:
- Around line 36-42: Add explicit negative test cases to enforce the [1,2)
invariant by checking is_normalised() returns false for values just below and
just above the normalized window: construct B test instances like B low{
FpType{0.75}, 0 } and B high{ FpType{2.0}, 0 } (matching the existing pattern
using FpType and B), then assert !low.is_normalised() and !high.is_normalised(),
incrementing nrFailures and printing the same tag-style message on failure;
place these checks alongside the existing norm and zero checks so the contract
for is_normalised() is fully validated.
In `@include/sw/universal/number/elreal/block_manipulators.hpp`:
- Around line 40-42: The code computes abs_frac then manually negates it when
negative; replace the manual sign check with a call to the standard absolute
function (e.g., use std::fabs(abs_frac) or std::abs(abs_frac)) so the assignment
to abs_frac uses the standard library for clarity and intent; update the
expression around the variable abs_frac in the block that constructs the
scientific string (the lines around abs_frac = static_cast<double>(b.v) /
std::ldexp(1.0, b.scale_of_v()) and the subsequent sign handling) to use
std::fabs/ std::abs instead of the if (abs_frac < 0) abs_frac = -abs_frac;
pattern.
🪄 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: 4de26393-296a-4f47-bc11-078da6f63797
📒 Files selected for processing (13)
CMakeLists.txtelastic/elreal/CMakeLists.txtelastic/elreal/block/accessors.cppelastic/elreal/block/construction.cppelastic/elreal/block/exp_offset.cppelastic/elreal/block/predicates.cppelastic/elreal/block/printers.cppelastic/elreal/block/zero_overlap.cppinclude/sw/universal/number/elreal/block.hppinclude/sw/universal/number/elreal/block_manipulators.hppinclude/sw/universal/number/elreal/elreal.hppinclude/sw/universal/number/elreal/elreal_fwd.hppinclude/sw/universal/number/elreal/exp_field_width.hpp
Comment on lines
+110
to
+126
| // is_normalised(): for IEEE-style FpTypes the leading significand bit must be | ||
| // set; equivalently, |v| in [1, 2). McCleeary blocks must be normalised so | ||
| // 0-overlap accounting holds. | ||
| constexpr bool is_normalised() const noexcept { | ||
| if (is_zero_block()) return false; | ||
| // For all supported FpType (native and Universal), ilogb / scale returns | ||
| // a valid integer for normalised values. Subnormals are NOT considered | ||
| // normalised here. | ||
| if constexpr (has_universal_fp_api_v<FpType>) { | ||
| // Universal cfloat / bfloat16: scale() works for both normal and | ||
| // subnormal values; check the value range explicitly. | ||
| FpType abs_v = v.sign() ? -v : v; | ||
| return abs_v >= FpType{1} || (v.scale() >= std::numeric_limits<FpType>::min_exponent - 1); | ||
| } else { | ||
| int cls = std::fpclassify(v); | ||
| return cls == FP_NORMAL; | ||
| } |
Contributor
There was a problem hiding this comment.
is_normalised() does not match documented block invariant
The implementation currently accepts values outside [1, 2) (for example 0.75 and 3.0), which conflicts with the stated normalized definition and can corrupt overlap assumptions.
Proposed fix
constexpr bool is_normalised() const noexcept {
if (is_zero_block()) return false;
- // For all supported FpType (native and Universal), ilogb / scale returns
- // a valid integer for normalised values. Subnormals are NOT considered
- // normalised here.
if constexpr (has_universal_fp_api_v<FpType>) {
- // Universal cfloat / bfloat16: scale() works for both normal and
- // subnormal values; check the value range explicitly.
FpType abs_v = v.sign() ? -v : v;
- return abs_v >= FpType{1} || (v.scale() >= std::numeric_limits<FpType>::min_exponent - 1);
+ return abs_v >= FpType{1} && abs_v < FpType{2};
} else {
- int cls = std::fpclassify(v);
- return cls == FP_NORMAL;
+ if (!std::isfinite(v)) return false;
+ const FpType abs_v = std::fabs(v);
+ return abs_v >= FpType{1} && abs_v < FpType{2};
}
}📝 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
| // is_normalised(): for IEEE-style FpTypes the leading significand bit must be | |
| // set; equivalently, |v| in [1, 2). McCleeary blocks must be normalised so | |
| // 0-overlap accounting holds. | |
| constexpr bool is_normalised() const noexcept { | |
| if (is_zero_block()) return false; | |
| // For all supported FpType (native and Universal), ilogb / scale returns | |
| // a valid integer for normalised values. Subnormals are NOT considered | |
| // normalised here. | |
| if constexpr (has_universal_fp_api_v<FpType>) { | |
| // Universal cfloat / bfloat16: scale() works for both normal and | |
| // subnormal values; check the value range explicitly. | |
| FpType abs_v = v.sign() ? -v : v; | |
| return abs_v >= FpType{1} || (v.scale() >= std::numeric_limits<FpType>::min_exponent - 1); | |
| } else { | |
| int cls = std::fpclassify(v); | |
| return cls == FP_NORMAL; | |
| } | |
| // is_normalised(): for IEEE-style FpTypes the leading significand bit must be | |
| // set; equivalently, |v| in [1, 2). McCleeary blocks must be normalised so | |
| // 0-overlap accounting holds. | |
| constexpr bool is_normalised() const noexcept { | |
| if (is_zero_block()) return false; | |
| if constexpr (has_universal_fp_api_v<FpType>) { | |
| FpType abs_v = v.sign() ? -v : v; | |
| return abs_v >= FpType{1} && abs_v < FpType{2}; | |
| } else { | |
| if (!std::isfinite(v)) return false; | |
| const FpType abs_v = std::fabs(v); | |
| return abs_v >= FpType{1} && abs_v < FpType{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 `@include/sw/universal/number/elreal/block.hpp` around lines 110 - 126, The
is_normalised() logic must enforce the documented invariant that |v| is in
[1,2); update both branches to test the absolute value range instead of (or in
addition to) fpclassify/scale: keep the early is_zero_block() check, compute
abs_v from v (using v.sign() ? -v : v) and return (abs_v >= FpType{1} && abs_v <
FpType{2}) for the universal FpType path (and do the same for the native path
instead of relying only on std::fpclassify(v) == FP_NORMAL); this ensures
is_normalised() only accepts values in [1,2) and rejects 0, subnormals and
values like 0.75 or 3.0.
Coverage Report for CI Build 26320320824Warning 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.03%) to 84.252%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
CodeRabbit findings - accessors.cpp: value_as<double>() comparison was effectively exact equality via `abs(diff) > 0.0`; switched to direct `!=` to make intent clear (value_as with offset=0 IS just a cast, no rounding expected). - block.hpp is_normalised(): the docstring claimed "|v| in [1,2)" which is wrong -- McCleeary normalisation is about the hidden bit being set (IEEE FP_NORMAL), independent of magnitude. 3.0 (= 1.5*2^1) IS normalised. Fixed the docstring and made the Universal-type branch use isnormal() via a C++20 requires-expression, falling back to a numeric_limits<>::min() check for types like bfloat16 that lack isnormal(). - predicates.cpp: added 0.75 and 3.0 as positive cases (both normal IEEE values, both must pass) and a subnormal case (must fail). Guards the subnormal test with has_denorm == denorm_present so it skips on formats without subnormal support. - block_manipulators.hpp: replaced the manual `if (x < 0) x = -x` with std::fabs() per CodeRabbit's nitpick. - exp_field_width.hpp: guarded the `1 << E` against undefined behaviour for hypothetical FpType with E >= 63 (none today, defensive only). Codacy finding - Cppcheck flagged `block::E` as unused (false positive -- it's a public constexpr referenced by the test). Restructured `block::exp_step` to compute `1 << E` locally instead of pulling from `exp_step_v<FpType>`, so Cppcheck sees the dependency. Same guard against E >= 63. Validation - gcc 13.3: all 6 tests still PASS (now with stricter coverage). - clang 18.1: all 6 tests still PASS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
include/sw/universal/number/elreal/block_manipulators.hpp (1)
67-74: 💤 Low valueEarly return bypasses
exp_offsetsuffix for Universal types.When
FpTypeis a Universal wrapper type (notfloatordouble),to_hexreturnsto_binary(b)directly at line 69, which does include theexp_offsetsuffix. So this is actually consistent, sinceto_binaryhandles it. However, the comment on line 68 says "defer to the human-readable form" without mentioning thatexp_offsetis handled there.Consider adding a brief comment clarifying that
to_binaryalready appendsexp_offset, or this could confuse future maintainers reading lines 71-73.🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp` around lines 67 - 74, The early return in to_hex that calls to_binary(b) for Universal wrapper FpType bypasses the later exp_offset suffix code, which is fine because to_binary already appends the exp_offset; update the to_hex implementation by replacing the vague comment "// Universal wrapper types: defer to the human-readable form for now." with a clear note that to_binary(b) includes the exp_offset suffix (mentioning b.exp_offset) so future readers understand why the subsequent exp_offset handling is skipped, or alternatively move the exp_offset-appending logic into a shared helper used by both to_hex and to_binary if you prefer a single source of truth.
🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp`:
- Around line 67-74: The early return in to_hex that calls to_binary(b) for
Universal wrapper FpType bypasses the later exp_offset suffix code, which is
fine because to_binary already appends the exp_offset; update the to_hex
implementation by replacing the vague comment "// Universal wrapper types: defer
to the human-readable form for now." with a clear note that to_binary(b)
includes the exp_offset suffix (mentioning b.exp_offset) so future readers
understand why the subsequent exp_offset handling is skipped, or alternatively
move the exp_offset-appending logic into a shared helper used by both to_hex and
to_binary if you prefer a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd8c04a0-74ff-4a09-9977-146614e5d198
📒 Files selected for processing (5)
elastic/elreal/block/accessors.cppelastic/elreal/block/predicates.cppinclude/sw/universal/number/elreal/block.hppinclude/sw/universal/number/elreal/block_manipulators.hppinclude/sw/universal/number/elreal/exp_field_width.hpp
✅ Files skipped from review due to trivial changes (1)
- elastic/elreal/block/predicates.cpp
CodeRabbit nitpick on PR #934: the comment in to_hex's Universal-wrapper branch said "defer to the human-readable form for now" without explaining that the early return is intentional because to_binary already appends the " ^ <exp_offset>" suffix. Expanded the comment so future readers don't add a redundant suffix to the bypass path. Comment-only change; no functional impact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
include/sw/universal/number/elreal/block_manipulators.hpp (4)
31-31: 💤 Low valueConsider
[[maybe_unused]]for unused parameters.The commented-out parameter
/*nibbleMarker*/suppresses unused-parameter warnings. The modern C++17 approach is[[maybe_unused]] bool nibbleMarker. Both are acceptable; this is a stylistic choice.🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp` at line 31, The parameter suppression in function to_binary(const block<FpType>& b, bool /*nibbleMarker*/ = false) should use the C++ attribute instead of a comment; update the signature for the function to_binary to declare the parameter as [[maybe_unused]] bool nibbleMarker = false (or equivalent C++17 attribute use) so the parameter name remains visible and unused-parameter warnings are suppressed; keep the default value and do not change other semantics.
53-55: 💤 Low valueConsider
[[maybe_unused]]for unused parameters.Similar to
to_binary, the/*nibbleMarker*/comment suppresses warnings. The C++17[[maybe_unused]]attribute is more idiomatic but the current approach is acceptable.🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp` around lines 53 - 55, The parameter comment trick in the to_hex function signature should be replaced with the C++17 [[maybe_unused]] attribute to mark nibbleMarker as intentionally unused; update the function declaration inline std::string to_hex(const block<FpType>& b, [[maybe_unused]] bool nibbleMarker = false, bool hexPrefix = true) so the compiler knows the parameter is unused without a comment and keeps the default behavior and signature semantics intact.
21-26: 💤 Low valueConsider implementation-dependent
typeid().name()output.The mangled name from
typeid(FpType).name()varies by compiler (e.g., "f" vs "float"). For diagnostic consistency, you might prefer a traits-based approach or explicit specializations, but the current implementation is acceptable for debugging purposes.🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp` around lines 21 - 26, The current type_tag(block<FpType>) uses implementation-dependent typeid(FpType).name(); replace this with a portable type-name trait: create a template type_name<T>() (or struct type_name<T>::get()) with explicit specializations for the common FpType candidates (e.g., float, double, long double, int types) and fallback to a generic demangled or "unknown" string, then change the type_tag implementation to use type_name<FpType>() instead of typeid(FpType).name() so diagnostics are consistent across compilers.
40-41: ⚖️ Poor tradeoffPotential edge cases in fraction extraction with extreme exponents.
Line 40 computes the fraction as
fabs(double(b.v) / ldexp(1.0, b.scale_of_v())). For extremescale_of_v()values (e.g., > 1023 or < -1074 for double),ldexpmay return infinity or zero, leading to0.0orinfin the output fraction. Additionally, converting wide FpTypes todoublemay lose precision.For Phase 1 diagnostic output this is likely acceptable, but consider guarding against overflow or documenting the limitation.
🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp` around lines 40 - 41, The current fraction computation using abs_frac = fabs(static_cast<double>(b.v) / std::ldexp(1.0, b.scale_of_v())) can produce 0.0 or inf for extreme b.scale_of_v() values and loses precision converting b.v to double; before calling std::ldexp(1.0, b.scale_of_v()) in this expression, guard by checking the exponent against double limits (std::numeric_limits<double>::max_exponent and min_exponent) or perform the scaling in long double, then validate the result with std::isfinite; if the scaled divisor is non-finite, set abs_frac to a safe sentinel (e.g., 0.0 or NAN) or emit a brief diagnostic note in the output so Phase 1 remains robust, and update the computation to use long double for intermediate (e.g., use std::ldexp((long double)1.0, b.scale_of_v()) and long double division) to reduce precision loss when converting b.v.
🤖 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 `@include/sw/universal/number/elreal/block_manipulators.hpp`:
- Line 31: The parameter suppression in function to_binary(const block<FpType>&
b, bool /*nibbleMarker*/ = false) should use the C++ attribute instead of a
comment; update the signature for the function to_binary to declare the
parameter as [[maybe_unused]] bool nibbleMarker = false (or equivalent C++17
attribute use) so the parameter name remains visible and unused-parameter
warnings are suppressed; keep the default value and do not change other
semantics.
- Around line 53-55: The parameter comment trick in the to_hex function
signature should be replaced with the C++17 [[maybe_unused]] attribute to mark
nibbleMarker as intentionally unused; update the function declaration inline
std::string to_hex(const block<FpType>& b, [[maybe_unused]] bool nibbleMarker =
false, bool hexPrefix = true) so the compiler knows the parameter is unused
without a comment and keeps the default behavior and signature semantics intact.
- Around line 21-26: The current type_tag(block<FpType>) uses
implementation-dependent typeid(FpType).name(); replace this with a portable
type-name trait: create a template type_name<T>() (or struct
type_name<T>::get()) with explicit specializations for the common FpType
candidates (e.g., float, double, long double, int types) and fallback to a
generic demangled or "unknown" string, then change the type_tag implementation
to use type_name<FpType>() instead of typeid(FpType).name() so diagnostics are
consistent across compilers.
- Around line 40-41: The current fraction computation using abs_frac =
fabs(static_cast<double>(b.v) / std::ldexp(1.0, b.scale_of_v())) can produce 0.0
or inf for extreme b.scale_of_v() values and loses precision converting b.v to
double; before calling std::ldexp(1.0, b.scale_of_v()) in this expression, guard
by checking the exponent against double limits
(std::numeric_limits<double>::max_exponent and min_exponent) or perform the
scaling in long double, then validate the result with std::isfinite; if the
scaled divisor is non-finite, set abs_frac to a safe sentinel (e.g., 0.0 or NAN)
or emit a brief diagnostic note in the output so Phase 1 remains robust, and
update the computation to use long double for intermediate (e.g., use
std::ldexp((long double)1.0, b.scale_of_v()) and long double division) to reduce
precision loss when converting b.v.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab783193-330a-46bd-aa7c-bd52acb0f5e8
📒 Files selected for processing (1)
include/sw/universal/number/elreal/block_manipulators.hpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/bugs/elreal-implementation.md`:
- Line 20: Wrap any inline formulas that contain asterisks in backticks or use a
fenced code/math block so Markdown doesn't treat * as emphasis; specifically
update the line containing "(x * y)(n) = round(x(k) * y(k), n)" (and the
similar occurrence at the other noted location) to be enclosed in backticks or a
math block, ensuring the entire formula is treated as code/math rather than
parsed text.
- Around line 14-205: The Markdown uses non-ASCII glyphs (e.g. arrows like "→",
comparison symbols "≥"/"≤", box-drawing characters in the table, quote bars, and
special symbols like "imp" and "ZBCL_k") which violates the ASCII-only rule;
update the document by replacing these with plain ASCII equivalents (use "->"
for arrows, ">="/"<=" for comparisons, plain pipe/+- style ASCII table borders
instead of box-drawing, plain quotes or ">" for quote bars, and ensure
identifiers like ZBCL_k, imp, elreal, lazy_component_buffer, at(k), c_k, ulp
etc. remain ASCII and unaltered), scan the entire file for any remaining Unicode
and convert to ASCII-safe forms while preserving meaning and formatting.
🪄 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: e0b27467-c9b5-43dd-94af-b9007e484b0c
📒 Files selected for processing (2)
CMakeLists.txtdocs/bugs/elreal-implementation.md
CodeRabbit findings on PR #934 (commit a1d0e61 added the notes file): 1. Unicode characters violate the project's no-Unicode rule (CLAUDE.md). Replaced throughout: section sign -> Sec.; superscripts -> ^N; sigma -> sum; en/em dashes -> --; right-arrow -> ->; logical AND -> "and"; filled-circle bullet -> *; left-half-block quote marker -> >; box-drawing characters -> proper Markdown table. 2. Markdown asterisks in inline pseudocode formulas (e.g. "(x * y)(n) = round(x(k) * y(k), n)") were being parsed as emphasis. Wrapped the two formula lines in a fenced code block so * is treated as literal punctuation. 3. While at it, fix the misspelled section heading "McCleearey algorithm" -> "McCleeary algorithm". No content changes; cosmetics + ASCII conformance only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9 tasks
This was referenced May 23, 2026
8 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
Phase 1 of the McCleeary LFPERA reimplementation (epic #923). Foundational
block<FpType>type with theint32_t exp_offsetescape multiplier, 0-overlap predicate, pretty-printers, and a sweep test suite over six FpType variants.(FpType v, int32_t exp_offset); trivial copyable, no in-class initE(b) = scale_of(v) + exp_offset * 2^E_FpType(int64, won't overflow)knumeric_limits<FpType>::digits(matches dissertation's block-vector width)float,double,half,bfloat16,cfloat<24,5>,cfloat<32,8>E(b1) >= E(b2) + k; zero blocks trivially 0-overlap any blockWhat's in / what's out
In Phase 1:
block<FpType>struct + accessors (sign(),scale_of_v(),exponent(),value_as<T>())is_zero_block(),is_normalised())zero_overlap(b1, b2)to_binary,to_hex,color_print)UNIVERSAL_BUILD_NUMBER_ELREALSoption, cascade into ELASTICS and CI_LITEDeferred:
k values observed
floatdoublehalf(cfloat<16,5>)bfloat16cfloat<24,5>cfloat<32,8>Note on
bfloat16: Universal'snumeric_limits<bfloat16>::digitsreports 7 (just the explicit fraction bits); the dissertation-consistent k would be 8 (including the hidden bit). Recorded here for visibility; not a Phase 1 blocker.Test results
el_block_constructionel_block_accessorsel_block_predicatesel_block_zero_overlapel_block_exp_offsetel_block_printersTest plan
gh pr ready) when satisfiedResolves #925
Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Build
Documentation