fix(@typegpu/gl): Evaluate the right-hand side of an aliasing const once - #3030
dchaudhari7177 wants to merge 2 commits into
Conversation
A const that aliases mutable memory evaluated its right-hand side three times: once in _constStatement, again when _aliasConstStatement hoisted the index accesses, and again when the rewritten access was resolved. Comptime code in the index therefore ran three times, so values[nextIndex()] read values[2] instead of values[0]. While a const statement is generated, cache the snippet each node of its right-hand side evaluates to, and reuse it on the later passes. Only those nodes are cached, since a function body generated along the way may be evaluated again with other argument types. Fixes software-mansion#3029
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- RHS single-evaluation for aliasing
consts —GlslGeneratornow overrides_constStatementand_expressionto memoize each right-hand-side node's snippet, so#hoistIndexAccessesand the final resolution reuse the first evaluation instead of re-running comptime code with side effects. collectObjectNodeshelper — records every array/object node in the RHS tree up front; identifiers (strings) are deliberately excluded so hoistedidxnames still resolve to their new definitions.- Nested-const safety — the active cache is saved and restored around
super._constStatementviatry/finally, and only RHS nodes are cached so shellless function bodies can still be regenerated with different argument types. - Two regression tests —
implicitPointer.test.tscovers the exact issue snippet (values[0].x,calls === 1) and a hoisted runtime index with a comptime part (int idx = (0 + index);,calls === 1).
I verified both new tests fail against the pre-fix generator (they produced values[2].x and (1 + index)), so they genuinely pin the bug. The full @typegpu/gl suite and pnpm --filter @typegpu/gl test:types pass locally. The fix is correctly scoped to GLSL, since the WGSL _aliasConstStatement does not re-evaluate the RHS.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
Abhirup0
left a comment
There was a problem hiding this comment.
Nice fix. Confining the RHS memoization strictly to GlslGenerator makes sense given that WGSL doesn't need to rewrite the AST for aliases.
A couple of quick checks on the implementation:
- The stack restoration via
previous = this.#constRhsin_constStatementis clean, but could we add a test for a case where comptime evaluation inside a const RHS executes another const declaration? That would give coverage for the stack handling intry/finally. - Have you verified multi-dimensional access (e.g.
grid[nextA()][nextB()])? Adding a case with two comptime indices on a nested structure would help ensure recursive hoisting handles the cached sub-nodes cleanly.
Other than those two test cases, the logic and scoping look solid to me.
|
Thanks @Abhirup0, both are good cases. Added in 7d218e0:
Against |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Review of the one commit added since the prior Pullfrog review (7d218e08e, test-only). Both new cases cover the scenarios raised in the human review and are meaningful — neutralizing the _constStatement/_expression overrides makes each of them fail.
- Nested-
constcache restore test —boids.$[pick() + nextIndex()], wherepick()declares its own aliasingconst, assertsint idx = (pick() + 1);andcalls === 2, pinning the save/restore of#constRhsacross a nested_constStatement. Pre-fix it emitspick() + 2. - Two-level hoisted access test —
grid[nextRow() + row]![nextCol() + col]!assertsint idx = (0 + row); int idx_1 = (0 + col);withrowCalls === 1andcolCalls === 1, exercising recursive#hoistIndexAccessesover multiple cached comptime indices. Pre-fix it emits(1 + row)/(1 + col).
pnpm vitest run tests/implicitPointer.test.ts → 11 passed. No new issues.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Fixes #3029.
Cause
For
const value = values[nextIndex()]!;the GLSL generator evaluated the right-hand side three times:WgslGenerator._constStatementevaluateseqNodeto decide the declaration kind. It lands in_aliasConstStatementbecause the value aliases a local array.#hoistIndexAccessesevaluates each index again to decide whether to hoist it. For a comptime index it keeps the original call node, since the result isconstant.this._expression(...)over the rewritten access evaluates that call node a third time.nextIndex()ran on each pass, and only the last result reached the output, so the shader readvalues[2].Fix
GlslGeneratornow overrides_constStatementand_expression. While aconstis generated, each node of its right-hand side keeps the snippet it evaluated to the first time, keyed by the node object. The hoisting walk and the final resolution get those snippets back instead of re-running the code.collectObjectNodes(eqNode)records the nodes up front, and_expressionconsults the cache only for those. A shellless function body generated while evaluating the RHS can be evaluated again with other argument types, so its nodes must not be cached. Nestedconststatements save and restore the previous cache.idxnames are new identifiers that must resolve to their new definitions.#hoistIndexAccessescreates are new arrays, so they are evaluated normally. Their children are the original nodes and come from the cache.The WGSL generator does not re-evaluate the RHS in
_aliasConstStatement, so it has no such problem, and this change stays inside@typegpu/gl.Tests
Two cases added to
packages/typegpu-gl/tests/implicitPointer.test.ts. Each asserts the generated GLSL andcalls === 1:return values[0].x;.boids.$[nextIndex() + index], hoists toint idx = (0 + index);. The comptime part runs once, and the runtime part is still hoisted to the declaration point.With only
src/glslGenerator.tsreverted, both new tests fail and the other 7 in the file pass.Also ran:
tsc --p ./tsconfig.json --noEmitandtsc --p ./tsconfig.test.json --noEmitfor@typegpu/gl: clean.oxlint -c oxlint.config.ts --max-warnings=0 --type-awareon both files: 0 warnings.oxfmt --checkon both files: clean.I did not run the full monorepo
pnpm test. The install here was filtered to@typegpu/gland its dependencies, and no test outsidepackages/typegpu-glimports it.