Skip to content

Retype matrix layout generic param from int to MatrixLayoutMode (for slang#12840) - #1135

Open
nv-slang-bot[bot] wants to merge 1 commit into
mainfrom
dev/slangpy-fixer/matrix-layout-mode
Open

Retype matrix layout generic param from int to MatrixLayoutMode (for slang#12840)#1135
nv-slang-bot[bot] wants to merge 1 commit into
mainfrom
dev/slangpy-fixer/matrix-layout-mode

Conversation

@nv-slang-bot

@nv-slang-bot nv-slang-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Status: Retype applied on dev/slangpy-fixer/matrix-layout-mode; promoted to ready-for-review by maintainer @jkwak-work (2026-09-04). ⚠️ Do not merge until the SGL_SLANG_VERSION pin bump lands (see Merge order) — on pinned Slang 2026.16.1 this hard-breaks SlangPy CI and every user on the pinned release.
  • Link: Companion to Fix bug in matrix specialization slang#12840 (the breaking change that requires this).
  • Verdict: Two SlangPy .slang extensions over matrix<T, R, C, L> declare the layout param as int; after slang#12840 retypes the built-in matrix layout param to the new MatrixLayoutMode enum they no longer type-check (E30019: expected 'MatrixLayoutMode', got 'int' — no implicit int→enum). Retyped both to MatrixLayoutMode.
  • Next-action: Validate the retype against #12840 with a local SGL_LOCAL_SLANG build (see Validation below), then execute the cross-repo Merge order.
  • Blocker: Circular CI dependency — this cannot pass SlangPy's own CI until the pinned Slang release contains MatrixLayoutMode; breaking the cycle needs one maintainer merge-gate override (see below).

Motivation

shader-slang/slang#12840 ("Fix bug in matrix specialization", labeled pr: breaking change) retypes the built-in matrix layout generic parameter from int to a new enum:

enum MatrixLayoutMode : int { Unknown, RowMajor, ColumnMajor }

// before: __generic<T = float, let R : int = 4, let C : int = 4, let L : int = ...>
// after:  __generic<T = float, let R : int = 4, let C : int = 4, let L : MatrixLayoutMode = MatrixLayoutMode.Unknown>
matrix<...>

Slang does not implicitly convert intMatrixLayoutMode, so any generic extension that binds the matrix layout param with let L : int fails to unify against the retyped matrix type. SlangPy has exactly two such sites (full-tree scan of origin/main), both compiled at module-load / dispatch time:

  • slangpy/slang/staticarray.slang:10ISizedArray extension over matrix<T, R, C, L>
  • src/sgl/device/print.slang:260IPrintable extension over matrix<T, R, C, L>

This is the sole cause of the red "SlangPy Tests" check on slang#12840. It is a type-check-time break, independent of the column-major codegen concern discussed on that PR.

Change

Retype the layout param let L : intlet L : MatrixLayoutMode at both sites. This mirrors exactly what slang#12840 does to its ~15 core-module matrix extension declarations. Coverage is preserved: the extension still binds the layout param generically, so it applies to all layout modes (unlike dropping the param, which would bind only the default Unknown layout).

MatrixLayoutMode is a core-module global enum, so it is visible in these modules with no additional import.

Validation (before merge, no merge needed)

The retype only compiles against a Slang that defines MatrixLayoutMode, which is unreleased (it lives in #12840). Validate this branch against #12840 with a local Slang build — the same mechanism CI uses (.github/actions/build-and-test-with-slang configures with -DSGL_LOCAL_SLANG=ON):

# In a Slang clone: fetch #12840 (works even though its branch is on a fork)
git fetch origin pull/12840/head && git checkout FETCH_HEAD   # or: gh pr checkout 12840
cmake --workflow --preset release        # build slang under build/Release

# In this SlangPy branch:
cmake --preset linux-gcc --fresh -DSGL_LOCAL_SLANG=ON \
      -DSGL_LOCAL_SLANG_DIR=<slang-dir> -DSGL_LOCAL_SLANG_BUILD_DIR=build/Release
cmake --build --preset linux-gcc-release
pytest slangpy/tests -v

Note: SlangPy's ci-latest-slang workflow_dispatch input is slang_branch, which uses branch checkout mode (git clone shader-slang/slang && git checkout <branch>) — it cannot reach matrix-layout-mode-enum, which lives only on the fknfilewalker/slang fork. The automatic repository_dispatch path uses pr checkout mode (git fetch origin pull/12840/head), which does resolve #12840 — but that path only ever builds SlangPy's default branch, not this PR branch. Hence the local build above is the reliable pre-merge validation.

Merge order (required — please read)

The "SlangPy Tests" check on slang#12840 is posted by SlangPy's ci-latest-slang.yml, which checks out SlangPy's default branch and builds it against the PR's Slang (pr mode). So that check only turns green once this retype is on main. But SlangPy's own CI (ci.yml) builds against the downloaded pinned release SGL_SLANG_VERSION = 2026.16.1 (external/CMakeLists.txt:95), which predates #12840 and has no MatrixLayoutMode — so this retype cannot pass SlangPy's own CI until that pin points at a Slang release containing the enum. Each repo's green gate depends on a state only the other repo can provide first: a genuine circular dependency that needs exactly one manual gate-override to break. Recommended sequence:

  1. Validate this branch against #12840 locally (see Validation above).
  2. Merge slang#12840 to master, overriding its red "SlangPy Tests" check — a known, expected, coordinated cross-repo break (this is the one manual override that breaks the cycle; the maintainer requested this companion PR precisely because of it).
  3. Cut a Slang release that includes #12840 (next tag after v2026.16.1).
  4. Land this PR with a pin bump: add SGL_SLANG_VERSION → the new release alongside this retype. SlangPy's own CI then downloads the enum-containing Slang, the retype compiles, and the check goes green.

(The pin bump is intentionally not in this PR yet — the target release does not exist, so bumping now would 404 the download. It must be added in step 4.)

If instead you prefer to green #12840's check before merging it, swap the order: admin-merge this retype PR to SlangPy main first (overriding SlangPy's own red ci.yml), then re-trigger "SlangPy Tests" on #12840 — merging SlangPy does not auto-rerun the previously-failed status; re-run it via the slang-side ci-slangpy-trigger-test.yml workflow_dispatch (pr_number=12840) or by pushing a new commit to #12840. This variant leaves SlangPy main red against the pinned 2026.16.1 until the pin bump (step 4) lands, so the sequence above is preferred.

Testing

No new test is added: this is a compatibility retype of two core .slang modules, both already exercised by the existing suite (staticarray.slang loads with the SlangPy module; print.slang compiles when the debug printer is used). The authoritative proof is the local SGL_LOCAL_SLANG build against #12840 (see Validation). That build was not run in this automated session (it requires the unreleased compiler plus a full Slang + SlangPy build); correctness here rests on exact correspondence to #12840's own core-module changes (identical intMatrixLayoutMode retype applied to ~15 matrix extension declarations) plus diff review. Please run the Validation build before promoting out of draft.

🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.

shader-slang/slang#12840 retypes the built-in matrix layout parameter
from `int` to the new `MatrixLayoutMode` enum (a breaking change). No
implicit int->enum conversion exists, so SlangPy's generic extensions
over `matrix<T, R, C, L>` that declare the layout param as `int` fail to
type-check against the new declaration (E30019: expected
'MatrixLayoutMode', got 'int').

Retype the layout param to `MatrixLayoutMode` at both sites so the
extensions unify against the retyped matrix type:
- slangpy/slang/staticarray.slang: ISizedArray extension
- src/sgl/device/print.slang: IPrintable extension
@jhelferty-nv

Copy link
Copy Markdown
Contributor

Automated notice (PR board sync) — do not reply to this comment.

Auto-assigned @kaizhangNV as shepherd for this Bot PR.

FYI for maintainers: committer signal on the changed files is highest for skallweitNV among collaborators other than the assignee. They were not auto-requested; a human may optionally add them as a reviewer.

@nv-slang-bot

nv-slang-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

CI on this draft is red as expected — this is the documented cross-repo blocker (see Merge order above), not a regression. The C++ unit tests compile src/sgl/device/print.slang against SlangPy's pinned Slang 2026.16.1, which predates the new enum:

src/sgl/device/print.slang:260 | __generic<T : IPrintable, let R : int, let C : int, let L : MatrixLayoutMode>
                                |   undefined identifier 'MatrixLayoutMode'.

MatrixLayoutMode exists only in shader-slang/slang#12840 (unreleased). This turns green once SGL_SLANG_VERSION is bumped to a Slang release that includes #12840 (step 4 of the merge order). Holding as draft. pre-commit, license/cla, and the aarch64 / x86_64-clang build legs pass.

🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.

@jkwak-work
jkwak-work marked this pull request as ready for review September 4, 2026 00:21
@jkwak-work
jkwak-work requested a review from a team as a code owner September 4, 2026 00:21

@fknfilewalker fknfilewalker 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.

LGTM, thats exactly how I fixed it for me locally.

@nv-slang-bot

nv-slang-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @fknfilewalker — confirmation that this matches your local fix is effectively the execution proof I couldn't produce in CI (SlangPy's own build can't compile it until the pinned SGL_SLANG_VERSION moves off 2026.16.1, which predates the enum).

Keeping this as a draft on purpose: merging to main before the pin is bumped to a Slang release that includes #12840 would hard-break SlangPy CI and everyone on the pinned release (undefined identifier 'MatrixLayoutMode'). Merge order is in the description — TL;DR: land #12840 → cut a Slang release with the enum → land this PR together with the SGL_SLANG_VERSION pin bump. I'll add that pin bump here as soon as the release tag exists.

🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.

@jkwak-work jkwak-work assigned jkwak-work and unassigned kaizhangNV Sep 4, 2026
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.

4 participants