impr: For implicit casts, break ties based on primitive preference, not order - #2978
impr: For implicit casts, break ties based on primitive preference, not order#2978reczkok wants to merge 3 commits into
Conversation
|
pkg.pr.new packages benchmark commit |
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 [0.86, 1.71, 3.69, 5.78, 5.87, 10.35, 21.14, 21.89]
line [0.83, 1.67, 3.51, 5.00, 6.67, 9.75, 19.88, 22.13]
line [0.89, 1.71, 3.06, 5.67, 6.02, 9.76, 19.28, 20.96]
---
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.24, 0.42, 0.58, 0.71, 1.02, 1.06, 1.20, 1.34]
line [0.22, 0.52, 0.69, 0.77, 1.07, 1.10, 1.28, 1.40]
line [0.25, 0.44, 0.62, 0.76, 1.11, 1.10, 1.26, 1.40]
---
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.84, 2.16, 3.05, 5.85, 10.10, 22.34, 48.86, 98.02]
line [0.75, 2.03, 2.99, 5.95, 10.41, 21.59, 46.68, 96.31]
line [0.79, 1.96, 3.10, 6.14, 10.27, 21.88, 48.02, 96.84]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.73%) | ❔ Unknown |
|---|---|---|---|
| 0 | 40 | 285 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| d_bool.ts | 14.75 kB ( |
| d_f16.ts | 14.75 kB ( |
| d_f32.ts | 14.75 kB ( |
| d_i32.ts | 14.75 kB ( |
| d_u32.ts | 14.75 kB ( |
| d_u16.ts | 14.78 kB ( |
| d_textureDepth2d.ts | 15.20 kB ( |
| d_textureDepthCube.ts | 15.20 kB ( |
| d_texture1d.ts | 15.21 kB ( |
| d_texture2d.ts | 15.21 kB ( |
| d_texture3d.ts | 15.21 kB ( |
| d_textureCube.ts | 15.21 kB ( |
| d_textureDepth2dArray.ts | 15.21 kB ( |
| d_textureDepthCubeArray.ts | 15.22 kB ( |
| d_textureDepthMultisampled2d.ts | 15.22 kB ( |
| d_texture2dArray.ts | 15.22 kB ( |
| d_textureCubeArray.ts | 15.23 kB ( |
| d_textureMultisampled2d.ts | 15.23 kB ( |
| tgpu_comptime.ts | 16.04 kB ( |
| std_discard.ts | 16.05 kB ( |
| std_extensionEnabled.ts | 16.26 kB ( |
| std_copy.ts | 16.30 kB ( |
| std_arrayLength.ts | 16.32 kB ( |
| std_isBeingTranspiled.ts | 16.57 kB ( |
| std_getShaderStage.ts | 16.63 kB ( |
| std_getTargetShaderLanguage.ts | 16.64 kB ( |
| std_range.ts | 16.53 kB ( |
| d_disarrayOf.ts | 16.70 kB ( |
| std_dpdx.ts | 17.02 kB ( |
| std_dpdxCoarse.ts | 17.02 kB ( |
| std_dpdxFine.ts | 17.02 kB ( |
| std_dpdy.ts | 17.02 kB ( |
| std_dpdyCoarse.ts | 17.02 kB ( |
| std_dpdyFine.ts | 17.02 kB ( |
| std_fwidth.ts | 17.02 kB ( |
| std_fwidthCoarse.ts | 17.02 kB ( |
| std_fwidthFine.ts | 17.02 kB ( |
| std_atomicLoad.ts | 17.82 kB ( |
| std_atomicStore.ts | 17.83 kB ( |
| std_textureBarrier.ts | 17.83 kB ( |
| std_atomicAdd.ts | 17.83 kB ( |
| std_atomicAnd.ts | 17.83 kB ( |
| std_atomicMax.ts | 17.83 kB ( |
| std_atomicMin.ts | 17.83 kB ( |
| std_atomicOr.ts | 17.83 kB ( |
| std_atomicSub.ts | 17.83 kB ( |
| std_atomicXor.ts | 17.83 kB ( |
| std_storageBarrier.ts | 17.83 kB ( |
| std_workgroupBarrier.ts | 17.83 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
There was a problem hiding this comment.
🟢 Approval recommended
The core ranking change is small and covered by updated snapshots plus an added regression test for order-independence; remaining feedback is limited to comment wording clarity.
Pull request overview
Improves the implicit-cast selection logic in getBestConversion so that when conversions are otherwise equivalent, the chosen target type is determined by the primitive preference ranking rather than the input order. This makes implicit conversion outcomes deterministic and aligned with the intended preference hierarchy.
Changes:
- Update implicit-cast ranking to incorporate destination primitive preference as a tie-breaker.
- Refresh snapshot expectations where the selected implicit cast target changes (e.g., array indices,
textureGatherarray index). - Add a regression test ensuring
getBestConversionresults are independent oftargetTypesordering.
File summaries
| File | Description |
|---|---|
| packages/typegpu/src/tgsl/conversion.ts | Adjusts implicit cast rank computation to incorporate primitive preference for deterministic tie-breaking. |
| packages/typegpu/tests/internal/getBestConversion.test.ts | Updates rank explanation comments and adds a test that asserts ordering-independence of targetTypes. |
| packages/typegpu/tests/tgsl/conversion.test.ts | Updates snapshot for index-access implicit conversion output. |
| packages/typegpu/tests/std/texture/textureGather.test.ts | Updates snapshot outputs to reflect new implicit cast choice for array index arguments. |
Review details
Suppressed comments (3)
packages/typegpu/tests/internal/getBestConversion.test.ts:77
- These examples say
dest < src, but what actually matters isdestPref < srcPref(preference rank). Using the exact condition makes the walkthrough match the code.
// u32 (3) -> f16 (1): dest < src => 10 + 1 = 11
// f16 (1) -> f16 (1): rank 0
// i32 (2) -> f16 (1): dest < src => 10 + 1 = 11
// Total Rank = 11 + 0 + 11 = 22
packages/typegpu/tests/internal/getBestConversion.test.ts:89
- Same as above: the condition is about preference (
destPref >= srcPref), not destination/source type ordering.
// f16 (1) -> u32 (3): dest >= src => 20 + 3 = 23
// i32 (2) -> u32 (3): dest >= src => 20 + 3 = 23
// Total Rank = 0 + 23 + 23 = 46
packages/typegpu/tests/internal/getBestConversion.test.ts:83
- These examples should reference
destPref >= srcPrefrather thandest >= src, since the ranking logic uses preference values.
// u32 (3) -> i32 (2): dest < src => 10 + 2 = 12
// f16 (1) -> i32 (2): dest >= src => 20 + 2 = 22
// i32 (2) -> i32 (2): rank 0
// Total Rank = 12 + 22 + 0 = 34
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The change delivers deterministic, order-independent implicit-cast target selection and the new test genuinely pins it. One constructive observation on how the tie-break is weighted is inline.
Reviewed changes
- conversion.ts:123 — implicit cast rank is now
(destPref < srcPref ? 10 : 20) + destPref, so equal-rank targets resolve by primitive preference (f32(0) > f16(1) > i32(2) > u32(3) > bool(4)) instead ofSetinsertion order. - getBestConversion.test.ts — updated the worked rank math in the existing comment and added an order-independence test that verifies
candidatesandcandidates.toReversed()yield the same target. - conversion.test.ts / textureGather.test.ts — regenerated index/conversion snapshots that now emit
i32instead ofu32for float indices, reflecting the new, order-free choice.
All three modified test files pass (25 tests). The new order-independence test is meaningful: under the old formula getBestConversion([f32, f32], [i32, u32]) lands on a rank-40 tie that resolves by insertion order, so the reversed-candidates assertion fails.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the commit is snapshots-only and consistent. Approved on the diff; the review is non-approving only because the weighting caveat from the prior review remains an open (non-blocking) thread.
Reviewed changes (delta since 433df77)
- function-visualizer.test.ts (typegpu-docs) — regenerated the docs example's array-index snapshot from
u32(...)toi32(...). This is exactly the expected consequence of the+ destPrefchange: array indexing usesrestrictTo [u32, i32](wgslGenerator.ts:693) with a float source that ties both candidates on base rank, and the new formula now deterministically prefersi32(pref 2 < 3). The source code is untouched.
Optional, non-blocking follow-up from the prior review (unchanged by this commit): the folded + destPref weights preference by operand count, which can overturn a strictly-better base target in 5–6-operand bool-mixing expressions under a multi-type restrictTo. A lexicographic tie-break (min base rank sum, then destPref) matches every PR test case and avoids that. Worth a decision before merge, not a blocker.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found in the incremental delta. The new commit cleanly resolves the prior caveat.
Reviewed changes
- conversion.ts — pure lexicographic tie-break. The
+ destPreffold (which scaled preference by operand count and could overturn a strictly-better base target at 5–6 operands) is replaced by a tie-break insidefindBestType: keep the lowest base-rank sum, and on an exactsum ===tie prefer the lowerdestPref. This is exactly the fix suggested in the prior review, and the concrete counter-examplegetBestConversion([u32, bool, bool, bool, bool], [f32, u32])now correctly returnsu32(sum 40 < f32's 50). - conversion.ts — module-scope hoist.
primitivePreference/getPreferencemoved out ofgetImplicitConversionRank;getPreferencereturnsNumber.POSITIVE_INFINITYfor non-primitives, so vector/struct ties keep the old insertion-order behavior. - getBestConversion.test.ts — order-independence tests. Reversed-candidates and reversed-sources assertions plus a regression test pinning the counterexample above. I simulated all four case families against the old and new comparators: the new logic matches every expectation, and the reversed-order assertions genuinely fail under base strict-
<behavior.
Verified: vitest on the three touched typegpu test files (27/27) and the docs function-visualizer snapshot test (1/1) all pass.
Prior feedback thread (conversion.ts:123 — + destPref weighting) is resolved by this commit; snapshot consistency from the earlier commit is preserved because the index [u32, i32] tie still resolves to i32 under the new rule.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
124d7bb to
1002c6b
Compare
Thankfully i decided to make the implicit conversion ranking system have some gaps so this is a small change (destPref < 10 is always true)