feat: Route function calls through the shader generator - #2800
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.93, 1.87, 4.30, 6.34, 7.63, 12.61, 22.61, 25.36]
line [0.97, 2.01, 4.13, 6.98, 7.30, 12.35, 24.10, 26.60]
line [0.99, 2.04, 4.50, 7.24, 8.60, 14.33, 22.36, 26.40]
---
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.69, 0.80, 1.06, 1.10, 1.34, 1.49]
line [0.31, 0.54, 0.68, 0.83, 1.23, 1.24, 1.43, 1.53]
line [0.36, 0.53, 0.73, 0.90, 1.26, 1.27, 1.51, 1.69]
---
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 [1.00, 2.11, 4.11, 6.83, 12.42, 27.86, 57.14, 114.01]
line [0.83, 2.06, 4.67, 7.16, 12.48, 26.73, 56.18, 117.10]
line [0.83, 2.14, 4.14, 7.35, 13.80, 28.25, 59.51, 122.76]
|
Bundle size comparison (
|
| 🟢 Decreased (max -0.28%) | ➖ Unchanged | 🔴 Increased (max 0.14%) | ❔ Unknown |
|---|---|---|---|
| 5 | 131 | 186 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 267.81 kB ( |
| tgpu_initFromDevice.ts | 267.28 kB ( |
| tgpu_resolve.ts | 168.58 kB ( |
| tgpu_resolveWithContext.ts | 168.51 kB ( |
| tgpu_bindGroupLayout.ts | 73.79 kB ( |
| tgpu_mutableAccessor.ts | 68.52 kB ( |
| tgpu_accessor.ts | 68.52 kB ( |
| tgpu_privateVar.ts | 67.21 kB ( |
| tgpu_workgroupVar.ts | 67.21 kB ( |
| tgpu_const.ts | 66.63 kB ( |
| tgpu_lazy.ts | 66.42 kB ( |
| tgpu_fragmentFn.ts | 38.92 kB ( |
| tgpu_fn.ts | 38.86 kB ( |
| tgpu_vertexFn.ts | 38.73 kB ( |
| tgpu_computeFn.ts | 38.44 kB ( |
| tgpu_vertexLayout.ts | 27.57 kB ( |
| tgpu_comptime.ts | 15.18 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.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one GLSL coverage gap to note inline.
Reviewed changes
This PR adds a call() method to ShaderGenerator, routes standard function calls (select, bitcast, arrayLength) through it, and overrides the method in GlslGenerator to translate select and bitcast into valid GLSL ES 3.0 constructs. It also threads the concrete return type into dualImpl codegen callbacks and adds snapshot tests for the GLSL output.
- Added
ShaderGenerator.call()and implemented it inWgslGenerator. - Overrode
call()inGlslGeneratorto translatebitcastandselectto GLSL equivalents. - Updated
dualImplto pass the concrete return type intocodegenImpl. - Migrated
arrayLengthand thebitcastfamily to usectx.gen.call(...). - Added GLSL snapshot tests for scalar/vector
selectandbitcast.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
| throw new Error(`Invalid number of arguments for 'select'`); | ||
| } | ||
|
|
||
| if (falsy.dataType !== UnknownData && falsy.dataType.type.startsWith('vec')) { |
There was a problem hiding this comment.
GLSL ES 3.0 only supports mix(genType, genType, genBType) for floating-point vectors, not for genIType or genUType. This means integer-vector std.select will still emit invalid GLSL here (it was already invalid before this PR, just via select(...)). Consider guarding non-float vector selects or adding a dedicated integer/unsigned path.
cieplypolar
left a comment
cieplypolar
left a comment
There was a problem hiding this comment.
Looks good. However, if someone ignores the select types, it will fail in the browser instead of during code generation.
333ec6b to
a8d9f1c
Compare
There was a problem hiding this comment.
ℹ️ No new issues — prior coverage gap remains open.
Reviewed changes
Re-reviewed the single commit now at the head of this PR. The diff still adds ShaderGenerator.call(), routes standard function calls through it, and overrides the method in GlslGenerator for bitcast and select. No new issues were introduced in this push.
- Added
ShaderGenerator.call()and implemented it inWgslGenerator. - Overrode
call()inGlslGeneratorto translatebitcastandselectto GLSL equivalents. - Updated
dualImplto pass the concrete return type intocodegenImpl. - Migrated
arrayLengthand thebitcastfamily to usectx.gen.call(...). - Added GLSL snapshot tests for scalar/vector
selectandbitcast.
The previously noted Pullfrog thread about GLSL integer-vector select emitting invalid mix(...) calls remains unresolved in this commit; it is still a documented coverage gap.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
@cieplypolar How so? 👀 |
a8d9f1c to
3a162b9
Compare
|
no API key found — this repo is configured to use To fix: add the key as a GitHub Actions secret (referenced from your workflow's Open repo secrets → · Configure model → · Setup docs → · Ask in Discord →
|
it('...', () => {
function foo() {
'use gpu';
const cond = d.vec3b(false, true, false);
// @ts-ignore
return std.select(0, 1, cond);
}
expect(tgpu.resolve([foo], glOptions())).toMatchInlineSnapshot(`
"int foo() {
bvec3 cond = bvec3(false, true, false);
return (cond ? 1i : 0i);
}"
`);
}); |
3a162b9 to
79e09df
Compare
79e09df to
0fa5849
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a ShaderGenerator.call(...) hook and routes selected standard-library function codegen through it, enabling backend-specific lowering (notably WGSL→GLSL) of calls like select, saturate, and bitcast.
Changes:
- Added
call(...)to theShaderGeneratorAPI and implemented it inWgslGenerator. - Updated
dualImplto pass the (concretized) return type intocodegenImpl, enabling call generation that depends on the return type (e.g.bitcast<T>(...)). - Refactored several std functions to use
ctx.gen.call(...), and added GLSL-specific lowering plus tests for standard function calls.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/src/tgsl/wgslGenerator.ts | Adds default call(...) emission for WGSL-style function calls (with optional template params). |
| packages/typegpu/src/tgsl/shaderGenerator.ts | Extends generator interface with call(...) to support backend-specific call routing. |
| packages/typegpu/src/std/numeric.ts | Routes std.saturate codegen through ctx.gen.call(...). |
| packages/typegpu/src/std/boolean.ts | Routes std.select codegen through ctx.gen.call(...) so backends can lower it differently. |
| packages/typegpu/src/std/bitcast.ts | Routes bitcast-related codegen through ctx.gen.call(...) and passes return type as a template param. |
| packages/typegpu/src/std/array.ts | Routes arrayLength(...) call emission through ctx.gen.call(...). |
| packages/typegpu/src/core/function/dualImpl.ts | Extends codegen signature to include return type and ensures it’s concretized once and reused. |
| packages/typegpu-gl/tests/glslGenerator.test.ts | Adds GLSL snapshot tests covering select, saturate, and bitcast lowering. |
| packages/typegpu-gl/src/glslGenerator.ts | Overrides call(...) to lower select/saturate/bitcast into GLSL equivalents. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Generating a ternary expression, which is supported in GLSL | ||
| return `(${this.ctx.resolveSnippet(cond).value} ? ${this.ctx.resolveSnippet(truthy).value} : ${this.ctx.resolveSnippet(falsy).value})`; | ||
| } |
There was a problem hiding this comment.
Important
The new saturate → clamp translation emits 0f/1f, which are not valid GLSL ES 3.0 float literals — std.saturate through @typegpu/gl will fail to compile in the browser.
Reviewed changes
Re-reviewed the delta since the prior Pullfrog review (a8d9f1cb). The branch was rebased onto a1a6d7e4; the only net change to the PR's own files is the new saturate routing.
- Routed
std.saturatethroughctx.gen.call('saturate', ...)instd/numeric.ts(WGSL output unchanged:saturate(x)). - Added a
saturatecase toGlslGenerator.call()that rewritessaturate(x)asclamp(x, 0f, 1f). - Added a GLSL snapshot test covering scalar and vector
saturate.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| if (!arg) { | ||
| throw new Error(`Invalid number of arguments for 'saturate'`); | ||
| } | ||
| return super.call('clamp', [], [arg, snip(0, d.f32, 'constant'), snip(1, d.f32, 'constant')]); |
There was a problem hiding this comment.
This emits clamp(x, 0f, 1f), but 0f/1f are not valid GLSL ES 3.0 literals — GLSL float constants require a decimal point (or exponent). Verified with glslang (Khronos reference): both clamp(float(v), 0f, 1f) and clamp(vec3, 0f, 1f) fail with "float literal needs a decimal point or exponent"; with 0.0/1.0 the same calls compile. The new snapshot test pins exactly this broken output, so std.saturate via @typegpu/gl will fail at shader-compile time at runtime.
| return super.call('clamp', [], [arg, snip(0, d.f32, 'constant'), snip(1, d.f32, 'constant')]); | |
| const [arg] = args; | |
| if (!arg) { | |
| throw new Error(`Invalid number of arguments for 'saturate'`); | |
| } | |
| return super.call( | |
| 'clamp', | |
| [], | |
| [arg, snip(0.0, d.f32, 'constant'), snip(1.0, d.f32, 'constant')], | |
| ); |
There was a problem hiding this comment.
frog is right about 0f and 1f constants, but sadly its fix won't work
| signature: unifyRestrictedSignature(anyFloat), | ||
| normalImpl: cpuSaturate, | ||
| codegenImpl: (_ctx, [value]) => stitch`saturate(${value})`, | ||
| codegenImpl: (ctx, [value]) => ctx.gen.call('saturate', [], [value]), |
There was a problem hiding this comment.
Routing saturate through ctx.gen.call('saturate', ...) is correct for the WGSL generator, but it newly enables the GLSL path above to emit invalid literals. If the GLSL translation stays this commit, the WGSL snapshots are unaffected (still saturate(x)), so the regression is confined to the GLSL generator.
0fa5849 to
d7d7a14
Compare
d7d7a14 to
59691a7
Compare
59691a7 to
601de13
Compare
| if (dataType.type.includes('3')) { | ||
| return d.vec3b; | ||
| } | ||
| return d.vec4b; |
There was a problem hiding this comment.
nit (just in case we accidentally use it in a function that accepts numbers)
| return d.vec4b; | |
| if (dataType.type.includes('4')) { | |
| return vec4b; | |
| } | |
| throw new Error( | |
| `Internal error: schema of type '${dataType.type}' does not have a corresponding boolean vector.`, | |
| ); |
| return snip(base, schema, /* origin */ 'constant', false); | ||
| } | ||
|
|
||
| public call(name: string, templateParams: readonly Snippet[], args: readonly Snippet[]): string { |
There was a problem hiding this comment.
Do we plan on routing all calls through here? Is there an issue for tracking that?
| normalImpl: cpuSelect, | ||
| codegenImpl: (ctx, [f, t, cond]) => { | ||
| const result = stitch`select(${f}, ${t}, ${cond})`; | ||
| const result = ctx.gen.call('select', [], [f, t, cond]); |
There was a problem hiding this comment.
I wonder if this is the best approach.
How about we aim for an api like this?:
// numeric.ts
export const dot = 1//...;
// numericGl.ts
polyfillGl(dot, (fn) => ({
signature: (...args) => {
if (isNumeric(args[0])) { throw new Error(...) }
return fn.signature(args);
},
codegenImpl: (_ctx, [lhs, rhs]) => stitch`dot(${lhs}, ${rhs})`, // or a ctx.gen.call
}))Or we could also try to extract the logic straight from glslgenerator to a list/map of polyfills.
No description provided.