impr: Clean up vectorOps - #2848
Conversation
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [1.00, 2.01, 4.45, 7.36, 8.70, 12.42, 23.89, 24.19]
line [0.95, 1.97, 4.14, 7.20, 8.36, 13.05, 25.46, 23.82]
line [0.97, 1.96, 4.33, 6.62, 8.02, 11.89, 25.95, 26.89]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.30, 0.50, 0.70, 0.82, 1.11, 1.19, 1.42, 1.57]
line [0.35, 0.54, 0.78, 0.85, 1.21, 1.15, 1.41, 1.53]
line [0.28, 0.53, 0.67, 0.81, 1.16, 1.29, 1.50, 1.55]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.96, 2.24, 3.55, 7.39, 12.87, 28.43, 58.05, 119.89]
line [0.93, 2.30, 3.96, 7.25, 12.97, 26.83, 57.13, 116.75]
line [0.97, 2.35, 4.12, 7.65, 13.01, 27.58, 60.04, 117.67]
|
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased (max -30.13%) | ➖ Unchanged | 🔴 Increased | ❔ Unknown |
|---|---|---|---|
| 116 | 209 | 0 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
Click to reveal the results table (116 entries).
| Test | tsdown |
|---|---|
| STATIC_allImports.ts | 298.33 kB ( |
| tgpu_resolveWithContext.ts | 272.64 kB ( |
| tgpu_bindGroupLayout.ts | 272.64 kB ( |
| tgpu_mutableAccessor.ts | 272.64 kB ( |
| tgpu_initFromDevice.ts | 272.64 kB ( |
| tgpu_vertexLayout.ts | 272.64 kB ( |
| tgpu_workgroupVar.ts | 272.64 kB ( |
| tgpu_fragmentFn.ts | 272.64 kB ( |
| tgpu_privateVar.ts | 272.64 kB ( |
| tgpu_computeFn.ts | 272.63 kB ( |
| tgpu_accessor.ts | 272.63 kB ( |
| tgpu_comptime.ts | 272.63 kB ( |
| tgpu_vertexFn.ts | 272.63 kB ( |
| tgpu_resolve.ts | 272.63 kB ( |
| tgpu_unroll.ts | 272.63 kB ( |
| tgpu_const.ts | 272.63 kB ( |
| tgpu_init.ts | 272.63 kB ( |
| tgpu_lazy.ts | 272.63 kB ( |
| tgpu_slot.ts | 272.63 kB ( |
| tgpu_fn.ts | 272.63 kB ( |
| STATIC_tgpu.ts | 272.63 kB ( |
| STATIC_std.ts | 97.39 kB ( |
| STATIC_d.ts | 77.38 kB ( |
| std_abs.ts | 56.26 kB ( |
| std_acos.ts | 56.26 kB ( |
| std_acosh.ts | 56.26 kB ( |
| std_asin.ts | 56.26 kB ( |
| std_asinh.ts | 56.26 kB ( |
| std_atan.ts | 56.26 kB ( |
| std_atan2.ts | 56.26 kB ( |
| std_atanh.ts | 56.26 kB ( |
| std_ceil.ts | 56.26 kB ( |
| std_clamp.ts | 56.26 kB ( |
| std_cos.ts | 56.26 kB ( |
| std_cosh.ts | 56.26 kB ( |
| std_countLeadingZeros.ts | 56.26 kB ( |
| std_countOneBits.ts | 56.26 kB ( |
| std_countTrailingZeros.ts | 56.26 kB ( |
| std_cross.ts | 56.26 kB ( |
| std_degrees.ts | 56.26 kB ( |
| std_determinant.ts | 56.26 kB ( |
| std_dot4I8Packed.ts | 56.26 kB ( |
| std_exp.ts | 56.26 kB ( |
| std_exp2.ts | 56.26 kB ( |
| std_extractBits.ts | 56.26 kB ( |
| std_faceForward.ts | 56.26 kB ( |
| std_firstLeadingBit.ts | 56.26 kB ( |
| std_firstTrailingBit.ts | 56.26 kB ( |
| std_floor.ts | 56.26 kB ( |
| std_fma.ts | 56.26 kB ( |
| std_insertBits.ts | 56.26 kB ( |
| std_intdiv.ts | 56.26 kB ( |
| std_inverseSqrt.ts | 56.26 kB ( |
| std_ldexp.ts | 56.26 kB ( |
| std_log.ts | 56.26 kB ( |
| std_log2.ts | 56.26 kB ( |
| std_max.ts | 56.26 kB ( |
| std_min.ts | 56.26 kB ( |
| std_normalize.ts | 56.26 kB ( |
| std_pow.ts | 56.26 kB ( |
| std_quantizeToF16.ts | 56.26 kB ( |
| std_radians.ts | 56.26 kB ( |
| std_reflect.ts | 56.26 kB ( |
| std_refract.ts | 56.26 kB ( |
| std_reverseBits.ts | 56.26 kB ( |
| std_round.ts | 56.26 kB ( |
| std_saturate.ts | 56.26 kB ( |
| std_sign.ts | 56.26 kB ( |
| std_sin.ts | 56.26 kB ( |
| std_sinh.ts | 56.26 kB ( |
| std_smoothstep.ts | 56.26 kB ( |
| std_sqrt.ts | 56.26 kB ( |
| std_step.ts | 56.26 kB ( |
| std_tan.ts | 56.26 kB ( |
| std_tanh.ts | 56.26 kB ( |
| std_transpose.ts | 56.26 kB ( |
| std_trunc.ts | 56.26 kB ( |
| std_distance.ts | 56.25 kB ( |
| std_dot4U8Packed.ts | 56.25 kB ( |
| std_fract.ts | 56.25 kB ( |
| std_frexp.ts | 56.25 kB ( |
| std_mix.ts | 56.25 kB ( |
| std_modf.ts | 56.25 kB ( |
| std_dot.ts | 56.25 kB ( |
| std_length.ts | 56.25 kB ( |
| std_rotateY4.ts | 39.00 kB ( |
| std_rotateZ4.ts | 39.00 kB ( |
| std_rotateX4.ts | 39.00 kB ( |
| std_scale4.ts | 39.00 kB ( |
| std_translate4.ts | 39.00 kB ( |
| std_add.ts | 38.17 kB ( |
| std_bitShiftLeft.ts | 38.17 kB ( |
| std_bitShiftRight.ts | 38.17 kB ( |
| std_div.ts | 38.17 kB ( |
| std_mod.ts | 38.17 kB ( |
| std_mul.ts | 38.17 kB ( |
| std_sub.ts | 38.17 kB ( |
| std_neg.ts | 38.16 kB ( |
| std_bitcast.ts | 35.92 kB ( |
| std_bitcastU32toF32.ts | 35.92 kB ( |
| std_bitcastU32toI32.ts | 35.92 kB ( |
| std_bitcastF32toU32.ts | 35.91 kB ( |
| std_ge.ts | 37.70 kB ( |
| std_gt.ts | 37.70 kB ( |
| std_isCloseTo.ts | 37.70 kB ( |
| std_le.ts | 37.70 kB ( |
| std_allEq.ts | 37.69 kB ( |
| std_eq.ts | 37.69 kB ( |
| std_lt.ts | 37.69 kB ( |
| std_ne.ts | 37.69 kB ( |
| std_not.ts | 37.69 kB ( |
| std_select.ts | 37.69 kB ( |
| std_and.ts | 37.69 kB ( |
| std_or.ts | 37.69 kB ( |
| std_all.ts | 37.69 kB ( |
| std_any.ts | 37.69 kB ( |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 263.10 kB ( |
| tgpu_initFromDevice.ts | 262.56 kB ( |
| tgpu_resolve.ts | 161.75 kB ( |
| tgpu_resolveWithContext.ts | 161.68 kB ( |
| tgpu_bindGroupLayout.ts | 62.32 kB ( |
| tgpu_mutableAccessor.ts | 57.04 kB ( |
| tgpu_accessor.ts | 57.04 kB ( |
| tgpu_privateVar.ts | 55.73 kB ( |
| tgpu_workgroupVar.ts | 55.73 kB ( |
| tgpu_const.ts | 55.15 kB ( |
| tgpu_lazy.ts | 54.95 kB ( |
| tgpu_fragmentFn.ts | 39.68 kB ( |
| tgpu_fn.ts | 39.62 kB ( |
| tgpu_vertexFn.ts | 39.50 kB ( |
| tgpu_computeFn.ts | 39.20 kB ( |
| tgpu_vertexLayout.ts | 28.33 kB ( |
| tgpu_comptime.ts | 15.93 kB ( |
| tgpu_unroll.ts | 1.75 kB ( |
| tgpu_slot.ts | 1.70 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
| } | ||
| throw new Error('Div called with invalid arguments.'); | ||
| verifyKind([lhs, rhs], numericKind); | ||
| return generalizeFn((a, b) => a / b, upCast([lhs, rhs])); |
There was a problem hiding this comment.
Previously integer vectors used a separate function for integer division. This works because vector constructors cast their elements.
|
@pullfrog review |
|
Run failed. View the logs →
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This cleanup introduces two CPU regressions in the refactored paths: the deprecated vector bitcasts now return the wrong type and corrupted values, and isCloseTo crashes on mixed scalar/vector operands where the codegen path returns false. Both are pinned inline with confirmation from running the CPU implementations directly.
Reviewed changes — large cleanup that removes most hand-written per-kind overloads in data/vectorOps.ts in favor of a new data/generalizeFn.ts helper (generalizeFn / generalizeBoolFn / upCast), which applies an n-ary scalar function component-wise over vectors/matrices and rebuilds the result via a kind→constructor map:
- New
generalizeFn.tswith theapplyArgsengine,kind/constructor maps,upCast, and the overloadedgeneralizeFn/generalizeBoolFnexports. std/numeric.tscpu*helpers (abs, acos, atan2, clamp, exp2, fract, log, max, min, mix, pow, sign, smoothstep, sqrt, trig, …) rewritten throughgeneralizeFn.std/operators.tsadd/mul/div/mod/negrewritten throughgeneralizeFn(div/mod viaupCast).std/boolean.tseq/lt/or/isCloseTo/selectrewritten viageneralizeBoolFn/generalizeFn.- Deprecated
bitcastU32toF32/bitcastU32toI32/bitcastF32toU32rewritten viageneralizeFn. VectorOpsreduced to the non-generalizable set (all,length,dot,cross, matrix multiply, bit shifts);clamprenamed toclampScalarinnumberOps.ts; tests updated for div-by-zero and distance.
ℹ️ Behavior change worth confirming
std.div (and std.mod) on integer vectors previously returned the operand on division by zero (divInteger returned lhs when rhs === 0); the generalized version now throws a Finite-Math-Assumption error via the u32/i32 cast. The rewritten test (forbids division by 0) intentionally encodes this, so it appears deliberate, but it is a silent behavioral change to the public CPU path. Worth confirming nothing downstream relies on the old lenient result.
ℹ️ Nitpicks
data/numberOps.tsstill exportsdivInteger, which is now referenced nowhere after this refactor — it should be removed with the other deleted helpers (bitcast*impls were removed in the same file).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
The isCloseTo crash fix (upcasting via upCast) resolves the CPU crash from the prior review but leaves a CPU/GPU divergence on mixed scalar/vector operands that the new test now locks in: the CPU computes a real component-wise result while the GPU codegen still returns the literal false.
Reviewed changes — incremental delta since the prior pullfrog review (9b5e1eb → cf3e123):
- Reverted the deprecated
bitcastU32toF32/bitcastU32toI32/bitcastF32toU32back to their originalVectorOps.*per-kind CPU impls, fixing the wrong-type/corrupted-value regression. - Removed the now-unreferenced
divIntegerhelper fromdata/numberOps.ts(also addresses the prior nitpick). - Fixed the
isCloseTomixed scalar/vector CPU crash by upcasting operands viaupCastbeforegeneralizeBoolFn, and expandedtests/std/boolean/isCloseTo.test.tswith close/distant/precision/mixed cases.
⚠️ CPU/GPU divergence on mixed isCloseTo
The new works for mixed types test asserts CPU results for mixed scalar/vector operands that a compiled shader cannot reproduce — the GPU codegen returns false for every mixed case while the CPU now computes the real comparison. Details inline.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestion inline.
Reviewed changes — incremental delta since the prior pullfrog review (cf3e123 → cfee895):
- Reverted the
upCast([lhs, rhs])call instd/boolean.tsisCloseTonormalImplback to passing[lhs, rhs]directly, returningisCloseToto uniform (same-kind) operands only — matching the thread reply to keep mixed operands out of scope. - Removed the
works for mixed typestest block fromisCloseTo.test.ts, dropping coverage of the scalar/vector mixed path.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — incremental delta since the last pullfrog review (8f4a717 → bfa48f5):
- Dropped the per-call array allocation in
applyArgs—['boolean', 'number'].includes(type)becametype === 'boolean' || type === 'number'; behaviorally identical. - Reworked
normalize's CPUnormalImpl—div(v, len)was replaced withgeneralizeFn((e) => e / len, [v]), dividing by the full-precisionlengthand moving the folded constant 1 ULP back to the pre-PRVectorOps.normalizevalue; a new exact-value regression test and a regenerated 3d-fish snapshot lock the behavior in.
ℹ️ The normalize exact-match test pins a reference device, not WGSL
WGSL's spec derives normalize(x) accuracy from x / length(x) (§15.7.4.1) without fixing exact bits, so adjacent-ULP results are all legitimate depending on how a backend lowers normalize. The pinned -0.2357022613286972 is x / length with length kept in full JS precision (the restored pre-PR behavior), while the superseded div(v, len) value -0.2357022762298584 divides by the f32-rounded length — what a literal f32 v / length(v) pipeline would produce. Both are within spec, so nothing to change; worth recording that CPU/GPU normalize parity is inherently reference-device-specific and the test shouldn't be read as universal WGSL equivalence.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
5e5d55f to
1fd63f3
Compare
There was a problem hiding this comment.
ℹ️ No new issues in the refactor — but the merge of
mainabsorbedstd.intdiv(#2849), and that function's CPU implementation diverges from both the WGSL it generates and this file's own zero-divisor convention. Worth reconciling, here or in the follow-up.
Reviewed changes — incremental delta since the prior pullfrog review (1fd63f3 → 6ad77a8), a merge of main:
- Absorbed
std.intdiv(#2849) intostd/numeric.ts(plus thestd/index.tsexport and newintdiv.test.ts) — the only change inside this PR's own files. The addition landed cleanly atop the refactor (usesdualImpl/unify/SignatureNotSupportedError, matching the file's new conventions) and all 38 focused tests pass (5intdivcases included). - Confirmed via
git diff 1fd63f3 HEADthat the remaining merged content — typegpu-cli Waku/Next.js template additions (#2599, #2596) — touches none of the PR's 10 files, and the refactoredvectorOps/generalizeFn/numberOps/booleansurface is byte-identical to what prior reviews covered.
ℹ️ Merged std.intdiv CPU path diverges from WGSL and from div's zero-divisor convention
intdiv is now the scalar half of this file's integer-division surface, and its CPU Math.trunc(Math.trunc(lhs) / Math.trunc(rhs)) disagrees with the WGSL lhs / rhs it emits on the two cases the spec defines explicitly: x / 0 (CPU yields Infinity/NaN; WGSL §8.7 defines a runtime result of the dividend x) and i32(-2147483648) / -1 (CPU yields 2147483648, outside i32; WGSL yields -2147483648). A compile-time 0 divisor is worse — the generated shader is rejected at creation while the CPU silently evaluates it. It's also inconsistent with div: the vector integer path deliberately turns a zero divisor into a loud Finite-Math-Assumption error (div.test.ts "forbids division by 0"), whereas intdiv returns a bogus float silently. Either matching WGSL's dividend (per the spec's own lowering hint) or throwing like div would close the gap.
Technical details
# std.intdiv CPU/GPU divergence on integer-division edge cases
## Affected sites
- packages/typegpu/src/std/numeric.ts:1215-1239 — `cpuIntdiv` / `intdiv` (absorbed from #2849 by the main merge; byte-identical to base `main`, so not anchorable on this PR's diff)
- packages/typegpu/src/std/operators.ts:208-219 — `div`'s CPU path (upCast + u32 schema conversion) is what throws the Finite-Math-Assumption error on zero divisors; `intdiv` has no equivalent guard
- packages/typegpu/tests/std/numeric/div.test.ts:51-57 — "forbids division by 0" pins the deliberate CPU error for the vector path
## Evidence
- WGSL spec §8.7 Arithmetic Expressions (https://www.w3.org/TR/WGSL/#arithmetic-expr): for integer `/`, a runtime (non-const) zero divisor is *defined* to evaluate to the dividend `e1`; the `e1 == most-negative-value && e2 == -1` case likewise evaluates to `e1`. A const-expression (or override-expression) zero divisor is a shader-creation (pipeline-creation) error.
- Corroborated by gpuweb/cts `i32_arithmetic.cache.ts` / `u32_arithmetic.cache.ts` (`division_nan` → `if (y === 0) return x`) and Tint `Eval::Div` runtime-semantics branch.
## Divergence table
| case | CPU `Math.trunc(lhs/rhs)` | WGSL runtime result |
|---|---|---|
| `intdiv(5, 0)` | `Infinity` | `5` (dividend) |
| `intdiv(-5, 0)` | `-Infinity` | `-5` |
| `intdiv(0, 0)` | `NaN` | `0` |
| `intdiv(-2**31, -1)` | `2147483648` (not i32-representable) | `-2147483648` |
| const 0 divisor | value evaluated silently | shader-creation error |
Truncated signed division (e.g. `-7 / 2 → -3`) agrees between JS and WGSL.
## Required outcome
CPU and generated-shader results agree for `intdiv` on (a) any zero divisor, (b) i32 MIN / -1.
## Suggested approach
Apply WGSL's own lowering hint (`Divisor = select(y, 1, (y==0) | (x==MIN && y==-1)); truncate(x/Divisor)`):
`const divisor = rhs === 0 || (lhs === -2147483648 && rhs === -1) ? 1 : rhs; return Math.trunc(lhs / divisor);`
For a zero divisor this returns the dividend, matching WGSL exactly. Alternatively throw on a zero divisor, consistent with `div`'s Finite-Math-Assumption error.
## Open questions for the human
Should a zero divisor THROW on the CPU (mirroring `div`'s Finite-Math-Assumption error) or return WGSL's defined dividend? Can be fixed here or folded into the next PR in the stack.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
No description provided.