Repository navigation
Fix FoR CastReduce kernel - #10169
Conversation
Signed-off-by: Matt Katz <[email protected]>
Signed-off-by: Matt Katz <[email protected]>
FoR casts whose references are out of rangeFoR casts whose values provably fit
Signed-off-by: Matt Katz <[email protected]>
FoR casts whose values provably fitFoR casts that change the integer type
Signed-off-by: Matt Katz <[email protected]>
Merging this PR will degrade performance by 10.79%
|
| 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)
Footnotes
-
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. ↩
Signed-off-by: Matt Katz <[email protected]>
FoR casts that change the integer typeFoR CastReduce kernel
robert3005
left a comment
There was a problem hiding this comment.
We could store the reference and dtype separately
|
@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 |
|
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 |
Summary
FoR's
CastReducenow 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 isencoded.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
developand, for the first two, on the commit before the blocked FoR stack (ba5fcfcde0^):[-128i8, 127]→i16[-128, 127][-128, -129][100i16, 200]→i8Err(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 decodeT::MAXgives wrong values.[-128i8, 127]has reference-128, so127is stored as255, which wraps to-1ini8. Casting the encoded child toi16sign-extends it to-1, which decodes as-128 + -1 = -129. Overi8values-128..=127, 128 of 256 values come out wrong, with no error.FoR::encodeproduces this whenever a signed column's range exceeds the type's maximum.[100i16, 200]cast toi8, the reference100and the offsets0and100each fiti8, but100 + 100doesn't, and the wrapping decode hides the overflow. Sign changes such asi8→u8andu8→i8fail the same way. The opposite also happens:-100i32..100fitsi8, but its offsets0..=199don't, so the cast fails on decode.5..10with reference-5can remain after filtering away the negatives. Cast tou32, the values fit but the reference doesn't.Constant'sCastReducedeclines, so the references slot is left holding a lazyvortex.cast, whichtry_new_chunkedaccepts because it only checks dtype and length. The cast appears to succeed, and decoding fails. Before BlockedFoRwith 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
CastReduceonly handles nullability, and its wideningCastKernelunpacks into a full-width buffer. A pushed-down widening cast therefore executes asFoR(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.FoR(Cast(BitPacked))can't use BitPacked's compare kernel.FoRSchemeto per-chunk references whenfastlanes.for.v2is allowed #10136,FoRSchemewrites per-chunk references whenfastlanes.for.v2is 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.