[DNI - DO NOT INTEGRATE] ci-benchmark: build against CUDA descriptor-array by-ref fix (slang#11939+#11941+#11940) - #1046
Conversation
…-typed fields Slang's CUDA target is gaining a by-reference ABI for entry-point uniform structs that carry descriptor tables (fixed-size arrays of resources or pointer-backed tensors): such a parameter reflects as an implicit ParameterBlock sub-object (shader-slang/slang#11774). The bound-variable recursion navigated into such a field with cursor[name] and passed the resulting cursor to children. Field lookups on a reference-typed cursor auto-dereference into the sub-object, so a child marshall caching those (sub-object-relative) offsets while writing through cursor.shader_object() - still the parent object - would silently corrupt memory. Dereference once at the recursion site so children always receive a cursor whose shader_object() owns the offsets they extract, matching what the ParameterBlock<CallData> fallback path already does for call_data. TensorMarshall and NativeTorchTensorMarshall additionally fail loudly (SGL_CHECK) if their own bound field is ever reference-typed, converting what would be silent corruption under a compiler/host version skew into a hard error. Tensor types themselves are never passed by reference, so the check never fires on supported shapes. This change is compatible with both current and by-reference Slang compilers (is_reference() is simply false everywhere today), and must land before slangpy bumps to a Slang containing the new ABI.
The vectorized-array path binds a generated Array1DValueType<T,N> struct wrapping the array. When T carries descriptors (tensors, buffers), Slang's CUDA by-reference ABI reflects that parameter as a ParameterBlock sub-object. NativeValueMarshall::ensure_cached navigated it with cursor[name]["value"]: the nested lookup auto-dereferences into the sub-object (making the cached offset sub-object-relative), while the write constructed a cursor on cursor.shader_object() - the parent object - producing out-of-bounds raw writes and heap corruption (test_vectorize_struct_with_tensor_array and friends on CUDA). Dereference explicitly when the bound field is reference-typed, cache the field index, and target the sub-object's ShaderObject at write time - the same pattern the call_data ParameterBlock fallback and the bound-variable recursion already use. No behavior change with compilers that pass everything by value (is_reference() is false everywhere).
📝 WalkthroughWalkthroughThis PR updates SlangPy marshalling to handle reference-typed shader fields, and adds a benchmark workflow that builds a local Slang fork and runs a new parameter-array benchmark suite. ChangesReference-typed field handling in shader marshalling
Local Slang benchmark workflow and parameter-array benchmark
Sequence Diagram(s)sequenceDiagram
participant ensure_cached
participant write_shader_cursor_pre_dispatch
ensure_cached->>ensure_cached: record field_is_reference and field_index
ensure_cached->>ensure_cached: dereference reference-typed field
write_shader_cursor_pre_dispatch->>write_shader_cursor_pre_dispatch: resolve sub-object ShaderObject
write_shader_cursor_pre_dispatch->>write_shader_cursor_pre_dispatch: build value_cursor against resolved object
Related Issues: None provided. Related PRs: None provided. Suggested labels: bug, shader-marshalling, ci, benchmark Suggested reviewers: None provided. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/format |
|
/format currently supports same-repository branches only. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5cc8180e-526b-448d-97ca-76a31b2980e7
📒 Files selected for processing (1)
.github/workflows/ci-benchmark.yml
…egalization shader-slang/slang#11939 rewrites a runtime index into a by-value kernel parameter array to go through an eager, whole-aggregate per-thread local copy. The copy is not made on demand: it is paid for the full array, per thread, per dispatch, regardless of how many elements are read. This benchmark sweeps the two axes that bound that trade - parameter size (16..512 floats) and dynamic accesses per thread (1 vs N) - plus a statically-indexed control where the legalization must not fire. Running it against a pre-#11939 build gives the serial .param-chain baseline; any pick_one regression on the new build quantifies the eager-copy tax and tells us whether the pass needs a size threshold.
…ly on current shader-slang#1045) Re-applies the DNI workflow (clone floor branch #11939+#11941, merge #11940, SGL_LOCAL_SLANG, --benchmark-save bypassing Mongo, pwsh on Windows, always-unlock) on top of the current host-safety shader-slang#1045 head + the cherry-picked param-array benchmark.
9fbe0b0 to
41d0967
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78d22675-d0e1-4718-84c6-58a3a5415d24
📒 Files selected for processing (4)
.github/workflows/ci-benchmark.ymlslangpy/benchmarks/test_benchmark_param_array.pyslangpy/benchmarks/test_benchmark_param_array.slangsrc/slangpy_ext/utils/slangpyvalue.cpp
| # DNI (do-not-integrate): build Slang from the descriptor-array by-reference fix. | ||
| # Composition under test (all three, matching real post-merge master): | ||
| # - shader-slang/slang#11939 dynamic-index local-copy legalization (the floor) | ||
| # - shader-slang/slang#11941 descriptor-table uniforms by reference (merged into #11939) | ||
| # - shader-slang/slang#11940 ParameterBlock-of-array emit-crash fix (independent) | ||
| # #11941 was merged into the floor branch (cuda-param-dynamic-index-floor), so that branch | ||
| # now carries BOTH #11939 and #11941; we clone it and fetch+merge #11940 so the implicit | ||
| # ParameterBlock-of-array this fix produces cannot hit the emit crash. | ||
| # NO slang-rhi bump here — unlike the retired F path (#779), the by-reference fix binds | ||
| # through the already-supported entry-point parameter-block sub-object path with zero | ||
| # host changes; SlangPy keeps its own external/slang-rhi pin (host-safety PR #1045). | ||
| - name: Build Slang (by-reference fix shader-slang/slang#11939+#11941 + #11940 — DNI) | ||
| run: | | ||
| git clone --recursive -b haaggarwal/cuda-param-dynamic-index-floor https://github.com/shader-slang/slang.git | ||
| cd slang | ||
| git config user.email "ci@slangpy.local" | ||
| git config user.name "slangpy-ci" | ||
| git fetch origin haaggarwal/cuda-pb-of-array-emit-fix | ||
| git merge --no-edit FETCH_HEAD | ||
| git submodule update --init --recursive | ||
| mkdir build | ||
| cmake -B build --preset default | ||
| cmake --build build --config ${{ matrix.config }} --parallel |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Build relies on mutable personal branches with no pinned commit.
Cloning haaggarwal/cuda-param-dynamic-index-floor and merging haaggarwal/cuda-pb-of-array-emit-fix by branch name (not commit SHA) means results aren't reproducible if either branch is force-pushed between runs, and git merge --no-edit FETCH_HEAD has no conflict handling — a conflict would abort the build with a raw git error. Given the explicit DNI/throwaway nature of this branch, this is likely an acceptable tradeoff for one-off data collection.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 96-96: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
| git submodule update --init --recursive | ||
| mkdir build | ||
| cmake -B build --preset default | ||
| cmake --build build --config ${{ matrix.config }} --parallel |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Static analysis flags template expansion of matrix.config as potential injection.
zizmor flags both ${{ matrix.config }} usages, but config is a fixed literal ([Release]) defined in this same workflow's matrix, not attacker-controllable input — unlike the github.run_id case fixed in the previous review round. Likely a false positive, but for consistency with the env-var pattern already adopted for RUN_ID, consider routing matrix.config through an env var too to keep the lint clean.
Also applies to: 122-122
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 96-96: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
Source: Linters/SAST tools
| - name: Benchmark (Python, Windows, GPU Clock Locked) | ||
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows' | ||
| run: python tools/ci.py benchmark-python --run-id "${{ github.run_id }}" --mongodb-connection-string "${{ secrets.BENCHMARK_MONGODB_CONNECTION_STRING }}" --mongodb-database-name "nvr-ci" --lock-gpu-clocks | ||
| env: | ||
| RUN_ID: ${{ github.run_id }} | ||
| run: | | ||
| python tools/gpu_clock.py lock --ratio 0.7 | ||
| python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-$env:RUN_ID-cuda"; $global:LASTEXITCODE = 0 | ||
| python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-$env:RUN_ID-vulkan"; $global:LASTEXITCODE = 0 | ||
| python -m pytest slangpy/benchmarks -ra --device-types d3d12 --benchmark-save "dni-$env:RUN_ID-d3d12"; $global:LASTEXITCODE = 0 | ||
| shell: pwsh | ||
|
|
||
| # Safety net: always release the GPU clocks on the shared self-hosted runner, even if the | ||
| # benchmark step above aborted — otherwise the clocks stay pinned at ratio 0.7 and corrupt | ||
| # every subsequent job on that runner. | ||
| - name: Unlock GPU clocks (Windows) | ||
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows' && always() | ||
| run: python tools/gpu_clock.py unlock | ||
| shell: pwsh | ||
|
|
||
| - name: Benchmark (Python, Linux, GPU Clock Unlocked) | ||
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Linux' | ||
| run: python tools/ci.py benchmark-python --run-id "${{ github.run_id }}" --mongodb-connection-string "${{ secrets.BENCHMARK_MONGODB_CONNECTION_STRING }}" --mongodb-database-name "nvr-ci" | ||
| env: | ||
| RUN_ID: ${{ github.run_id }} | ||
| run: | | ||
| python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-${RUN_ID}-cuda" || true | ||
| python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-${RUN_ID}-vulkan" || true | ||
| shell: bash |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Benchmark step failures are fully swallowed — no CI signal if all devices fail.
Every pytest invocation resets the exit code unconditionally ($global:LASTEXITCODE = 0 on Windows, || true on Linux), regardless of whether it succeeded or failed. This is more than "don't abort mid-step" — it means the step (and thus the job) will report success even if every single device benchmark crashes, with no JSON artifacts produced. Given the entire point of this DNI branch is to reliably collect RTX 5090 numbers, a silent all-device failure could go unnoticed unless someone reads the raw log closely.
Consider capturing/reporting per-device exit status (e.g., emit a ::warning:: annotation, or fail the step only if zero device types produced output) so a total failure is visible in the job status rather than only in the log text.
Example: surface failures instead of fully swallowing them
- name: Benchmark (Python, Windows, GPU Clock Locked)
if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows'
env:
RUN_ID: ${{ github.run_id }}
run: |
python tools/gpu_clock.py lock --ratio 0.7
- python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-$env:RUN_ID-cuda"; $global:LASTEXITCODE = 0
- python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-$env:RUN_ID-vulkan"; $global:LASTEXITCODE = 0
- python -m pytest slangpy/benchmarks -ra --device-types d3d12 --benchmark-save "dni-$env:RUN_ID-d3d12"; $global:LASTEXITCODE = 0
+ python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-$env:RUN_ID-cuda"
+ if ($LASTEXITCODE -ne 0) { Write-Host "::warning::cuda benchmark failed" }
+ $global:LASTEXITCODE = 0
+ python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-$env:RUN_ID-vulkan"
+ if ($LASTEXITCODE -ne 0) { Write-Host "::warning::vulkan benchmark failed" }
+ $global:LASTEXITCODE = 0
+ python -m pytest slangpy/benchmarks -ra --device-types d3d12 --benchmark-save "dni-$env:RUN_ID-d3d12"
+ if ($LASTEXITCODE -ne 0) { Write-Host "::warning::d3d12 benchmark failed" }
+ $global:LASTEXITCODE = 0
shell: pwsh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Benchmark (Python, Windows, GPU Clock Locked) | |
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows' | |
| run: python tools/ci.py benchmark-python --run-id "${{ github.run_id }}" --mongodb-connection-string "${{ secrets.BENCHMARK_MONGODB_CONNECTION_STRING }}" --mongodb-database-name "nvr-ci" --lock-gpu-clocks | |
| env: | |
| RUN_ID: ${{ github.run_id }} | |
| run: | | |
| python tools/gpu_clock.py lock --ratio 0.7 | |
| python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-$env:RUN_ID-cuda"; $global:LASTEXITCODE = 0 | |
| python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-$env:RUN_ID-vulkan"; $global:LASTEXITCODE = 0 | |
| python -m pytest slangpy/benchmarks -ra --device-types d3d12 --benchmark-save "dni-$env:RUN_ID-d3d12"; $global:LASTEXITCODE = 0 | |
| shell: pwsh | |
| # Safety net: always release the GPU clocks on the shared self-hosted runner, even if the | |
| # benchmark step above aborted — otherwise the clocks stay pinned at ratio 0.7 and corrupt | |
| # every subsequent job on that runner. | |
| - name: Unlock GPU clocks (Windows) | |
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows' && always() | |
| run: python tools/gpu_clock.py unlock | |
| shell: pwsh | |
| - name: Benchmark (Python, Linux, GPU Clock Unlocked) | |
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Linux' | |
| run: python tools/ci.py benchmark-python --run-id "${{ github.run_id }}" --mongodb-connection-string "${{ secrets.BENCHMARK_MONGODB_CONNECTION_STRING }}" --mongodb-database-name "nvr-ci" | |
| env: | |
| RUN_ID: ${{ github.run_id }} | |
| run: | | |
| python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-${RUN_ID}-cuda" || true | |
| python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-${RUN_ID}-vulkan" || true | |
| shell: bash | |
| - name: Benchmark (Python, Windows, GPU Clock Locked) | |
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows' | |
| env: | |
| RUN_ID: ${{ github.run_id }} | |
| run: | | |
| python tools/gpu_clock.py lock --ratio 0.7 | |
| python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-$env:RUN_ID-cuda" | |
| if ($LASTEXITCODE -ne 0) { Write-Host "::warning::cuda benchmark failed" } | |
| $global:LASTEXITCODE = 0 | |
| python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-$env:RUN_ID-vulkan" | |
| if ($LASTEXITCODE -ne 0) { Write-Host "::warning::vulkan benchmark failed" } | |
| $global:LASTEXITCODE = 0 | |
| python -m pytest slangpy/benchmarks -ra --device-types d3d12 --benchmark-save "dni-$env:RUN_ID-d3d12" | |
| if ($LASTEXITCODE -ne 0) { Write-Host "::warning::d3d12 benchmark failed" } | |
| $global:LASTEXITCODE = 0 | |
| shell: pwsh | |
| # Safety net: always release the GPU clocks on the shared self-hosted runner, even if the | |
| # benchmark step above aborted — otherwise the clocks stay pinned at ratio 0.7 and corrupt | |
| # every subsequent job on that runner. | |
| - name: Unlock GPU clocks (Windows) | |
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Windows' && always() | |
| run: python tools/gpu_clock.py unlock | |
| shell: pwsh | |
| - name: Benchmark (Python, Linux, GPU Clock Unlocked) | |
| if: contains(matrix.flags, 'benchmark') && runner.os == 'Linux' | |
| env: | |
| RUN_ID: ${{ github.run_id }} | |
| run: | | |
| python -m pytest slangpy/benchmarks -ra --device-types cuda --benchmark-save "dni-${RUN_ID}-cuda" || true | |
| python -m pytest slangpy/benchmarks -ra --device-types vulkan --benchmark-save "dni-${RUN_ID}-vulkan" || true | |
| shell: bash |
| # Upload the saved benchmark JSON(s) so the medians survive even if the log is trimmed. | ||
| - name: Upload benchmark reports | ||
| if: contains(matrix.flags, 'benchmark') && always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: benchmark-reports-${{ matrix.os }} | ||
| path: .benchmarks/**/*.json | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Unpinned action reference flagged by static analysis.
actions/upload-artifact@v4 isn't pinned to a commit hash, which zizmor reports as a required-policy error.
Pin to a commit SHA
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa9 # v4.6.2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Upload the saved benchmark JSON(s) so the medians survive even if the log is trimmed. | |
| - name: Upload benchmark reports | |
| if: contains(matrix.flags, 'benchmark') && always() | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: benchmark-reports-${{ matrix.os }} | |
| path: .benchmarks/**/*.json | |
| if-no-files-found: warn | |
| # Upload the saved benchmark JSON(s) so the medians survive even if the log is trimmed. | |
| - name: Upload benchmark reports | |
| if: contains(matrix.flags, 'benchmark') && always() | |
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa9 # v4.6.2 | |
| with: | |
| name: benchmark-reports-${{ matrix.os }} | |
| path: .benchmarks/**/*.json | |
| if-no-files-found: warn |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 168-168: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
Source: Linters/SAST tools
| def _make_inputs(device: spy.Device, size: int): | ||
| weights = np.random.rand(size).astype(np.float32) | ||
| indices = np.random.randint(0, size, size=CALL_SHAPE).astype(np.uint32) | ||
| indices_tensor = spy.Tensor.from_numpy(device, indices) | ||
| result_tensor = spy.Tensor.empty(device, shape=CALL_SHAPE, dtype=float) | ||
| return weights, indices, indices_tensor, result_tensor |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Missing return type annotation on _make_inputs.
Flagged by Ruff (ANN202). Low priority since it's a private test helper, not exported API surface.
Add return type
-def _make_inputs(device: spy.Device, size: int):
+def _make_inputs(
+ device: spy.Device, size: int
+) -> tuple[np.ndarray, np.ndarray, spy.Tensor, spy.Tensor]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _make_inputs(device: spy.Device, size: int): | |
| weights = np.random.rand(size).astype(np.float32) | |
| indices = np.random.randint(0, size, size=CALL_SHAPE).astype(np.uint32) | |
| indices_tensor = spy.Tensor.from_numpy(device, indices) | |
| result_tensor = spy.Tensor.empty(device, shape=CALL_SHAPE, dtype=float) | |
| return weights, indices, indices_tensor, result_tensor | |
| def _make_inputs( | |
| device: spy.Device, size: int | |
| ) -> tuple[np.ndarray, np.ndarray, spy.Tensor, spy.Tensor]: | |
| weights = np.random.rand(size).astype(np.float32) | |
| indices = np.random.randint(0, size, size=CALL_SHAPE).astype(np.uint32) | |
| indices_tensor = spy.Tensor.from_numpy(device, indices) | |
| result_tensor = spy.Tensor.empty(device, shape=CALL_SHAPE, dtype=float) | |
| return weights, indices, indices_tensor, result_tensor |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 37-37: Missing return type annotation for private function _make_inputs
(ANN202)
Source: Linters/SAST tools
| weights = np.random.rand(size).astype(np.float32) | ||
| indices = np.random.randint(0, size, size=CALL_SHAPE).astype(np.uint32) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
No fixed RNG seed — non-reproducible failures.
np.random.rand/np.random.randint are unseeded, so a flaky/failing assertion can't be reproduced from the same test invocation. Consider a fixed seed for debuggability, since reproducibility doesn't affect the perf numbers being measured.
|
Closing — throwaway DNI branch, its purpose is done. Used it to collect the RTX 5090 (Linux+Windows) before/after benchmark numbers for the CUDA descriptor-array by-reference fix (slang#11939+#11941 + #11940), which are captured on #11939. Not for integration. Deleting the branch. 🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify. |
2 participants
Do-not-integrate: temporarily wire the scheduled CUDA benchmark to build SlangPy against the CUDA descriptor-array by-reference fix, to get RTX 5090 numbers on the nvrgfx perf runners before the upstream PRs land. Throwaway — DO NOT MERGE.
Composition under test
123279b6, this branch's base — NativeValueMarshall reference-aware).What the workflow does
haaggarwal/cuda-param-dynamic-index-floor(now carries both #11939 and #11941) and merges #11940.SGL_LOCAL_SLANG=ONagainst that build. No slang-rhi bump — the by-ref fix binds through the existing entry-point parameter-block sub-object path with zero host changes.test_tensor_sum_indirecton Windows + Linux nvrgfx perf runners → nvr-ci Mongo.Local L40S validation (already green, #1030 heuristic OFF)
test_array.py -k cuda: 14 passed / 1 metal-skip / 0 crash (the 3 previously-corrupting vectorized-tensor-array tests now pass).ld.globalnot.paramchain.This PR exists to confirm the same on the RTX 5090 regression hardware.
🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.