Skip to content

Fix FoR CastReduce kernel - #10169

Merged
mhk197 merged 5 commits into
developfrom
mk/for-cast-reference
Oct 1, 2026
Merged

mhk197 merged 5 commits into
developfrom
mk/for-cast-reference

Conversation

@mhk197

@mhk197 mhk197 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

FoR's CastReduce now pushes down only casts that change nullability (eq_ignore_nullability). Every cast that changes the integer type declines, so the array decodes and the decoded values are cast. This fixes three correctness bugs in casting FoR arrays. Two of them have been there since before the new FoR array.

Background

FoR stores each value as an offset from a reference: encoded = value.wrapping_sub(reference) in the source type, and decoding is encoded.wrapping_add(reference) in whatever type the array has. The encoded values are therefore offsets modulo 2ⁿ, stored in the source type, and not ordinary values.

The old kernel cast the encoded child and the references to the target type separately, then rebuilt the FoR array. That treats the offsets as plain values, which is only correct when every offset fits the source type as a non-negative number and every decoded value fits the target type.

Bugs

Reproduced on develop and, for the first two, on the commit before the blocked FoR stack (ba5fcfcde0^):

Cast Expected Got
[-128i8, 127] → i16 [-128, 127] [-128, -129]
[100i16, 200] → i8 Err(value exceeds target range) [100, -56]
5..10 (reference -5) → u32 [5, 6, 7, 8, 9] Err(No CastReduce to cast constant array from i32 to u32) on decode
  1. Widening a signed type whose range exceeds T::MAX gives wrong values. [-128i8, 127] has reference -128, so 127 is stored as 255, which wraps to -1 in i8. Casting the encoded child to i16 sign-extends it to -1, which decodes as -128 + -1 = -129. Over i8 values -128..=127, 128 of 256 values come out wrong, with no error. FoR::encode produces this whenever a signed column's range exceeds the type's maximum.
  2. Narrowing wraps instead of erroring. In [100i16, 200] cast to i8, the reference 100 and the offsets 0 and 100 each fit i8, but 100 + 100 doesn't, and the wrapping decode hides the overflow. Sign changes such as i8 → u8 and u8 → i8 fail the same way. The opposite also happens: -100i32..100 fits i8, but its offsets 0..=199 don't, so the cast fails on decode.
  3. An out-of-range reference leaves an undecodable array. Values 5..10 with reference -5 can remain after filtering away the negatives. Cast to u32, the values fit but the reference doesn't. Constant's CastReduce declines, so the references slot is left holding a lazy vortex.cast, which try_new_chunked accepts because it only checks dtype and length. The cast appears to succeed, and decoding fails. Before Blocked FoR with one reference per 1024-element chunk #10105, the scalar reference cast failed eagerly, so the whole cast errored instead. Per-chunk references hit the same issue, because a slice keeps the references of every chunk it overlaps.

The existing tests didn't catch these: every conformance case has a small range and widens.

Why decline instead of proving the cast safe

  • The pushdown doesn't save work. BitPacked's CastReduce only handles nullability, and its widening CastKernel unpacks into a full-width buffer. A pushed-down widening cast therefore executes as FoR(Cast(BitPacked)): it unpacks into a wide buffer, then adds the reference in a second pass, because the child is no longer BitPacked and FoR's fused unpack-and-add path doesn't apply. Declining gives one fused decode in the narrow type, then a widening primitive cast. The pass count is the same, with the fused pass over narrower data.
  • Nothing downstream benefits. For example, a compare on FoR(Cast(BitPacked)) can't use BitPacked's compare kernel.
  • It wouldn't apply to per-chunk references. Since Refine FoRScheme to per-chunk references when fastlanes.for.v2 is allowed #10136, FoRScheme writes per-chunk references when fastlanes.for.v2 is allowed. The proof needs the minimum and maximum of the references child, which may be lazy inside a reduce rule, so every type change would decline there anyway.

A nullability-only cast is always safe: it changes neither the values nor the non-nullable references, so it is still pushed down, for single and per-chunk references alike.

@mhk197 mhk197 changed the title Decline FoR casts whose references are out of range Only push down FoR casts whose values provably fit Sep 30, 2026
@mhk197 mhk197 changed the title Only push down FoR casts whose values provably fit Fix FoR casts that change the integer type Oct 1, 2026
@mhk197 mhk197 added the changelog/fix A bug fix label Oct 1, 2026
@codspeed

codspeed Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.79%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

❌ 1 regressed benchmark
✅ 2084 untouched benchmarks
⏩ 503 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ WallTime dict_canonicalize_gt_u8_neon[1000000] 488.9 µs 548.1 µs -10.79%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing mk/for-cast-reference (5ff71e0) with develop (14aed5c)

Open in CodSpeed

Footnotes

  1. 503 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@mhk197
mhk197 marked this pull request as ready for review October 1, 2026 00:32
@mhk197 mhk197 changed the title Fix FoR casts that change the integer type Fix FoR CastReduce kernel Oct 1, 2026

@robert3005 robert3005 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could store the reference and dtype separately

@mhk197

mhk197 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

@robert3005 what would that fix?

I think solution is probably (at least for for-bp) to have a for/bp kernel that adds then casts as it streams so just 1 copy

@mhk197
mhk197 merged commit e555530 into develop Oct 1, 2026
101 of 102 checks passed
@mhk197
mhk197 deleted the mk/for-cast-reference branch October 1, 2026 14:14
@robert3005

Copy link
Copy Markdown
Contributor

It just makes casts lazy and correct so you can keep things as FoR after casting but really this is not the right place to keep things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants