Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The comptime probe can incorrectly classify array expressions and prune runtime-dependent branches.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds std.isKnownAtComptime for compile-time branching during shader generation, with exports, tests, and documentation.
Changes:
- Implements and exports the new utility.
- Adds coverage for literals, runtime values, arrays, side effects, and simulation.
- Documents usage and loop-unrolling integration.
| File | Summary |
|---|---|
packages/typegpu/tests/std/comptime.test.ts |
Adds behavioral and regression test coverage. |
packages/typegpu/src/std/index.ts |
Exports the new utility. |
packages/typegpu/src/std/comptime.ts |
Implements the comptime probe. Critical (1 vote): array expressions can be misclassified when containing runtime values, allowing incorrect branch pruning; add composite-value handling and a regression test. Nit (3 votes): rephrase the grammatically incorrect “it being able” sentence. |
apps/typegpu-docs/src/content/docs/apis/utils.mdx |
Documents the API and usage. Nit (3 votes): rephrase the grammatically incorrect “it being able” sentence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| return snip( | ||
| isSnippetKnownAtComptime(value), |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
std.isKnownAtComptime throws for valid, side-effect-free arguments. The guard keys off Snippet.possibleSideEffects, which is deliberately conservative ("unable to reliably determine it doesn't have side effects" also sets it), so an argument that is genuinely comptime-known can still be rejected. Confirmed repro below — this should be fixed (or the guard narrowed) before merge.
Reviewed changes
- New
std.isKnownAtComptime(packages/typegpu/src/std/comptime.ts) — customDualFn; returnstruein JS, and during codegen inspectsisKnownAtComptime(arg)to return aboolconstant snippet. - Export (
packages/typegpu/src/std/index.ts) — wired into thestdbarrel. - Docs (
apps/typegpu-docs/.../apis/utils.mdx) — new section plus atgpu.unrolltip linking to it. - Tests (
packages/typegpu/tests/std/comptime.test.ts) — 9 tests covering JS, literals, fn args, fixed/runtime array length, unroll selection, side-effect throw, stored side-effect, andsimulate. All pass locally.
Technical details
# `std.isKnownAtComptime` rejects side-effect-free arguments
## Affected sites
- `packages/typegpu/src/std/comptime.ts:83-87` (new) — throws whenever `value.possibleSideEffects` is true.
- `packages/typegpu/src/tgsl/wgslGenerator.ts:1744` — the unroll element index is built as `snip(i, u32, 'constant')` with no 4th argument, so `possibleSideEffects` defaults to `true` (`packages/typegpu/src/data/snippet.ts:158`).
- `packages/typegpu/src/tgsl/accessIndex.ts:55` — propagates that flag to the element snippet.
## Repro (verified against this branch)
```ts
const arr = tgpu.accessor(d.arrayOf(d.u32, 3), [10, 20, 30]); // comptime default
const f = () => {
'use gpu';
let acc = d.u32();
for (const x of tgpu.unroll(arr.$)) {
acc += std.isKnownAtComptime(x) ? 1 : 0; // throws
}
return acc;
};
tgpu.resolve([f]);
```
Each `x` is the comptime value `10/20/30`, so `true` is the correct answer, yet resolution fails with `\`isKnownAtComptime\` received an argument with possible side effects`. Unrolling a fixed buffer-backed array (`tgpu.unroll(layout.$.boids)`) fails the same way, where the correct answer is `false`. The error message's premise ("a side-effectful expression is never known at comptime anyway, so the result would always be `false`") is therefore not accurate here.
## Required outcome
- `std.isKnownAtComptime` must not throw for arguments whose evaluation is side-effect-free but which merely carry the conservative `possibleSideEffects` flag.
- Add a regression test exercising the guard inside an unrolled loop.
## Suggested approach (optional)
- Tighten the producer: pass `/* possibleSideEffects */ false` for the synthesized constant index at `wgslGenerator.ts:1744` (and audit similar `snip(..., 'constant')` sites), or
- Narrow the guard to only reject arguments that are definitely side-effectful rather than merely not provably pure.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| if (value.possibleSideEffects) { | ||
| throw new WgslTypeError( | ||
| '`isKnownAtComptime` received an argument with possible side effects. The expression is never emitted into the shader, so its side effects would silently not happen, and a side-effectful expression is never known at comptime anyway, so the result would always be `false`. Remove the check, and run the side effect as a separate statement if it is needed.', |
There was a problem hiding this comment.
This guard throws for arguments that are side-effect-free but carry the conservative possibleSideEffects flag. Snippet.possibleSideEffects is documented as "sure it has side effects OR unable to reliably determine it doesn't", so valid code like for (const x of tgpu.unroll(arr.$)) { std.isKnownAtComptime(x) } over a comptime array throws, even though x is comptime-known. See the review body for a verified repro and fix options.
04c0623 to
b869b07
Compare
b869b07 to
51872b1
Compare
Implements #3027