feat(arithmetic): add SubtractWiden at every tier - #20
Conversation
AddKernel has AddWiden at 32->64, 64->128 and 128->256, and MultiplyKernel, DivideKernel and ModulusKernel all have a *Widen, but Subtract had none. A caller wanting an exact subtraction in the next tier up had to promote both operands by hand and subtract at the wider width, which is correct but leaves the reader to convince themselves the promotion cannot change the answer. Subtraction needs widening for the same reason addition does: the result scale is max(s1,s2) for both, so both can overflow the operand width before the rescale brings the result back down. Add SubtractWiden to AddKernel at all three tier steps, and to SpanAddKernel alongside its AddWiden overloads. The span 32->64 same-scale path gets a SIMD helper mirroring AddWidenSameScale32To64: widening cannot overflow the width, so it carries no overflow accumulator and only enforces the declared precision, on both halves of each widened pair. Closes #14
There was a problem hiding this comment.
Pull request overview
Adds SubtractWiden APIs for scalar and span-based decimal arithmetic across 32→64, 64→128, and 128→256 tiers.
Changes:
- Added scalar and span widening subtraction implementations.
- Added SIMD, precision, ordering, and equivalence tests.
- Added range-check coverage for vector and scalar-tail paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Review summary |
|---|---|
tests/Clast.DatabaseDecimal.Tests/SpanAddKernelTests.cs |
No final comment supplied. |
tests/Clast.DatabaseDecimal.Tests/FusedRangeCheckTests.cs |
No final comment supplied. |
tests/Clast.DatabaseDecimal.Tests/Decimal256Tests.cs |
No final comment supplied. |
tests/Clast.DatabaseDecimal.Tests/ArithmeticTests.cs |
No final comment supplied. |
src/Clast.DatabaseDecimal/Arithmetic/SpanAddKernel.cs |
Critical: widening span overloads do not preserve the documented overlap guarantee. |
src/Clast.DatabaseDecimal/Arithmetic/AddKernel.cs |
No final comment supplied. |
Suppressed comments (2)
src/Clast.DatabaseDecimal/Arithmetic/SpanAddKernel.cs:486
- This 64→128 widening overload also writes each
Int128result before laterlonginput elements have been read. A result span overlapped with an input span, which the enclosing class documents as supported, therefore clobbers future inputs and returns incorrect values. Please stage overlapping inputs or explicitly reject/document overlap for widening operations.
public static void SubtractWiden(
ReadOnlySpan<long> left, DecimalType leftType,
ReadOnlySpan<long> right, DecimalType rightType,
Span<Int128> result, DecimalType resultType,
src/Clast.DatabaseDecimal/Arithmetic/SpanAddKernel.cs:506
- This 128→256 widening overload has the same overlap failure: writing a wider result element in the forward loop can overwrite
Int128input elements that subsequent iterations still need, despite the enclosing class documenting overlapping result/input spans as safe. Please stage overlapping inputs or explicitly reject/document overlap for widening operations.
public static void SubtractWiden(
ReadOnlySpan<Int128> left, DecimalType leftType,
ReadOnlySpan<Int128> right, DecimalType rightType,
Span<Int256> result, DecimalType resultType,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public static void SubtractWiden( | ||
| ReadOnlySpan<int> left, DecimalType leftType, | ||
| ReadOnlySpan<int> right, DecimalType rightType, | ||
| Span<long> result, DecimalType resultType, |
There was a problem hiding this comment.
Checked this one carefully, and I do not think it is a defect in this PR — but it did catch an imprecise doc line, which I have now fixed (b8c8e0e).
Two reasons the code is not wrong:
-
Not introduced here. The three
SubtractWidenoverloads are structurally identical to theAddWidenoverloads directly above them — same shape, same loop, sign flipped. If widening broke the overlap guarantee, it broke it in 0.3.0 whenAddWidenshipped. This PR mirrors that behaviour rather than changing it. -
The overlap it describes is not reachable by ordinary use. The result span is a different element type from the inputs (
Span<long>vsReadOnlySpan<int>), so the two cannot alias without a deliberateMemoryMarshal.Castof one buffer into both types. There is noresult === leftcall a caller can write here the way they can for the same-width overloads.
The deeper point is that a widening operation has no in-place formulation at all: each result element is twice the width of its inputs, so an in-place widening write has nowhere to put the second half. Staging inputs would not rescue an aliased call — it would just make a physically impossible operation silently produce something.
So the fix is on the documentation side, not the implementation: the class summary asserted the overlap guarantee unconditionally, which over-promised for every widening overload including the pre-existing AddWiden ones. It now scopes the guarantee to the same-width overloads and explains why widening is excluded.
The class summary promised a safe result/input overlap without qualification, but a widening overload writes an element twice the width of its inputs, so there is no in-place formulation for it to honour. Say so, and note that overlapping differently-typed spans takes a deliberate MemoryMarshal reinterpretation in the first place.
SubtractWiden arrived in #20 while the folded-mask overloads were in flight in #21, so the two crossed: every other add and subtract entry point can hand back a per-row out-of-range mask and the widening subtract could not. Nothing about it is special — it was simply not there to extend at the time. Adds the three overloads, one per widening step, so the mask surface is symmetric across add and subtract again. The 32-to-64 one gets a vectorised masked helper matching its bool-returning sibling; the other two are scalar loops, as the plain overloads are. The widening SIMD path is the fiddly one to get right, because each 32-bit chunk widens into two 64-bit vectors and so contributes mask bits for two half-chunks, at b0 and b0 + halfLanes. Swapping those two offsets still produces plausible-looking output, so the tests were checked against exactly that mutation and five of the eleven lengths fail under it — the ones long enough to reach the vector path. Benchmarks gain the widening pair, whose per-step lane bookkeeping is heavier than the same-width kernels already covered. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes #14.
AddKernelhadAddWidenat 32→64, 64→128 and 128→256, andMultiplyKernel,DivideKernelandModulusKerneleach have a*Widen, butSubtracthad none — so a widening subtract meant promoting both operands by hand and subtracting at the wider width.Subtraction needs widening for the same reason addition does: the result scale is
max(s1,s2)for both, so both operands can overflow the operand width before the rescale brings the result back down.What changed
AddKernel— three newSubtractWidenoverloads, each the sign-flipped twin of theAddWidenbeside it:Decimal32 - Decimal32 → Decimal64(Widen32To64)Decimal64 - Decimal64 → Decimal128(Widen64To128)Decimal128 - Decimal128 → Decimal256(Widen128To256)SpanAddKernel— the same gap existed in the columnar API, where the by-hand workaround is worse (you have to materialize widened spans first), soSubtractWidenlands there too, at all three tier steps. The 32→64 same-scale path gets a SIMD helper mirroringAddWidenSameScale32To64: widening cannot overflow the width, so like its add twin it carries no overflow accumulator and only enforces the declared precision — on both halves of each widened pair.Signatures, defaults (
HalfEven/Throw), and the fused range-check behaviour match theAddWidenoverloads exactly. Purely additive; nothing existing changed.Tests
NUMERIC(38,0) - NUMERIC(38,0) → NUMERIC(39,0)case from the issue.SubtractWidenvs. the promote-both-operands-first workaround over a grid of mantissas at mixed scales, plus the same equivalence assertion at 128→256 — so the claim that the workaround was correct is now checked rather than argued.ThrowandIgnoreat the scalar level, and both the SIMD vector body and the scalar tail for the span 32→64 path (followingFusedRangeCheckTests' length-65 convention).Full suite passes on net8.0, net10.0, and net472 — 564 tests, 0 failures (up from 549).
Note on the alternative
The issue offered documenting the omission as deliberate instead. I took the API route: the workaround is only obviously equivalent if you already know rescaling is monotone in the mantissa, and the columnar case has no comfortable one-liner at all.