Skip to content

fix: Prevent tgpu.unroll from introducing possible side effects - #3237

Merged
iwoplaza merged 9 commits into
mainfrom
fix/unroll-possible-side-effects
Oct 23, 2026
Merged

iwoplaza merged 9 commits into
mainfrom
fix/unroll-possible-side-effects

Conversation

@cieplypolar

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI balanced review requested due to automatic review settings October 21, 2026 09:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

@cieplypolar cieplypolar changed the title fix: Prevent tgpu.unroll from introducing possible side effects fix: Prevent tgpu.unroll from introducing possible side effects Oct 21, 2026
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

pkg.pr.new

packages
Ready to be installed by your favorite package manager ⬇️

https://pkg.pr.new/software-mansion/TypeGPU/typegpu@217113bb3e3fd00bf7cb07ba0e861047627c9407
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/gl@217113bb3e3fd00bf7cb07ba0e861047627c9407
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/react@217113bb3e3fd00bf7cb07ba0e861047627c9407
https://pkg.pr.new/software-mansion/TypeGPU/unplugin-typegpu@217113bb3e3fd00bf7cb07ba0e861047627c9407

benchmark
view benchmark

commit
view commit

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

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.49, 1.01, 2.14, 3.31, 3.73, 6.13, 12.18, 12.12]
  line [0.48, 0.98, 2.21, 3.46, 4.30, 6.70, 12.04, 13.79]
  line [0.50, 1.05, 1.95, 2.85, 3.76, 6.16, 12.16, 11.31]
Loading
---
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.15, 0.29, 0.36, 0.43, 0.58, 0.61, 0.68, 0.77]
  line [0.14, 0.23, 0.33, 0.43, 0.54, 0.58, 0.70, 0.78]
  line [0.17, 0.29, 0.37, 0.41, 0.59, 0.57, 0.70, 0.80]
Loading
---
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.47, 1.20, 2.07, 3.71, 6.66, 13.34, 28.75, 58.14]
  line [0.48, 1.08, 2.08, 3.69, 6.55, 13.70, 29.63, 59.09]
  line [0.44, 1.20, 2.17, 3.69, 6.68, 13.70, 29.57, 58.40]
Loading

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Bundle size comparison (import * as ... in PR vs import * as ... in target):

🟢 Decreased ➖ Unchanged 🔴 Increased (max 0.00%) ❔ Unknown
0 349 6 0

No notable changes.

If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

The production fix is correct, but the new regression test does not compile and fails at resolution time: std.isKnownAtComptime is not a real API. CI (test:unit / test:types) will be red until the test is rewritten.

Reviewed changes

  • wgslGenerator.ts unrolled loop index — the constant loop index is now created with snip(i, u32, 'constant', false) instead of the default possibleSideEffects: true; since accessIndex propagates target.possibleSideEffects || index.possibleSideEffects, this stops the flag from leaking into every unrolled element.
  • unroll.test.ts regression test — a new does not introduce possibleSideEffects test using an accessor array; see the inline comment, as it references a function that does not exist.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread packages/typegpu/tests/unroll.test.ts
@cieplypolar
cieplypolar force-pushed the fix/unroll-possible-side-effects branch from 837fa2a to fb70b2e Compare October 21, 2026 09:48
@cieplypolar
cieplypolar changed the base branch from main to feat/std-is-known-at-comptime October 21, 2026 09:49

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The PR was rebased onto feat/std-is-known-at-comptime since the last review; the fix and test content are otherwise unchanged.

  • Unrolled-loop index side effects — the constant loop index is still built with snip(i, u32, 'constant', false), so accessIndex no longer propagates possibleSideEffects onto every unrolled element.
  • Regression test now valid — retargeting the base makes std.isKnownAtComptime available, and that builtin throws on an argument with possibleSideEffects. Verified locally: the test passes with the fix and fails when line 1864 is reverted to snip(i, u32, 'constant'), so it genuinely pins the fix and the previous blocker is resolved.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@cieplypolar
cieplypolar force-pushed the fix/unroll-possible-side-effects branch from fb70b2e to b65c973 Compare October 21, 2026 09:52
@iwoplaza
iwoplaza force-pushed the feat/std-is-known-at-comptime branch from 2c7e842 to 4c0964d Compare October 21, 2026 10:00
@cieplypolar
cieplypolar force-pushed the fix/unroll-possible-side-effects branch from b65c973 to 9f1b40f Compare October 21, 2026 10:01
@iwoplaza
iwoplaza force-pushed the feat/std-is-known-at-comptime branch from 4c0964d to d08b2b9 Compare October 21, 2026 10:03
@cieplypolar
cieplypolar force-pushed the fix/unroll-possible-side-effects branch from 9f1b40f to e5ebb3a Compare October 21, 2026 10:04
@iwoplaza
iwoplaza force-pushed the feat/std-is-known-at-comptime branch from d08b2b9 to 654963b Compare October 21, 2026 10:40
Base automatically changed from feat/std-is-known-at-comptime to main October 21, 2026 10:50

@iwoplaza iwoplaza left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just rebase on top of main

@iwoplaza
iwoplaza merged commit fece4b2 into main Oct 23, 2026
7 of 8 checks passed
@iwoplaza
iwoplaza deleted the fix/unroll-possible-side-effects branch October 23, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants