Default slang-test compiler invocations to -O0#11805
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds default optimization handling for ChangesDefault slang-test optimization handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jkwak-work
force-pushed
the
disable-spv-opt-in-slang-test
branch
from
f270d60 to
968453a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a81fff05-4c13-4ed6-b887-a77409d4d901
⛔ Files ignored due to path filters (1)
tests/bugs/vk-structured-buffer-load.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (42)
tests/autodiff/differentiable-interface-return.slangtests/autodiff/reverse-loop-immediate-return.slangtests/bugs/gh-10774-concrete-return.slangtests/bugs/gh-6482-interface-method-existential-specialize.slangtests/bugs/gh-9916.slangtests/bugs/metal-return-value-lost.slangtests/bugs/spirv-opt-SROA-of-globals.slangtests/compute/byte-address-buffer-array.slangtests/compute/half-texture.slangtests/compute/nonuniformres-as-function-parameter.slangtests/compute/nonuniformres-nested-rwstructuredbuf.slangtests/compute/static-const-array.slangtests/cooperative-vector/layout-structuredbuffer.slangtests/cross-compile/loop-attribs.slangtests/cross-compile/precise-keyword.slangtests/diagnostics/command-line/option-missing-argument.slangtests/glsl-intrinsic/intrinsic-basic-vector.slangtests/glsl-intrinsic/intrinsic-texture.slangtests/glsl/matrix-bool-lowering.slangtests/gpu-feature/texture/query/footprint/nv-shader-texture-footprint.slangtests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-chit.slangtests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-miss.slangtests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-rgen.slangtests/hlsl/texture-mips-operator.slangtests/language-feature/dynamic-dispatch/layout-array-2d.slangtests/language-feature/dynamic-dispatch/layout-array-field.slangtests/language-feature/dynamic-dispatch/layout-array-of-structs.slangtests/language-feature/dynamic-dispatch/layout-array-of-vectors.slangtests/language-feature/execution-model/varying-array-input.slangtests/language-feature/stage-switch.slangtests/language-feature/swizzles/matrix-swizzle-out-arg-writeback.slangtests/llvm/printf.slangtests/optimization/arrray-storage-lowering.slangtests/optimization/buffer-load-defer-bindless.slangtests/optimization/defer-structured-buffer-load.slangtests/pipeline/ray-tracing/ray-query-subroutine.slangtests/pipeline/ray-tracing/trace-ray-inline.slangtests/spirv/abort-defer.slangtests/spirv/mesh-shader-invert-y.slangtests/vkray/anyhit.slangtools/slang-test/README.mdtools/slang-test/slang-test-main.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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: 6bfb9b5c-f93b-4a95-8d91-89401fc40065
📒 Files selected for processing (5)
tests/wgsl/matrix-bool-lowering.slangtools/slang-test/README.mdtools/slang-test/slang-test-main.cpptools/slang-test/slang-test-optimization-options.htools/slang-unit-test/unit-test-slang-test-optimization-options.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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: 4a8b9f6c-1ba2-4b6b-bff3-05cd465e9653
📒 Files selected for processing (3)
tools/slang-test/README.mdtools/slang-test/slang-test-optimization-options.htools/slang-unit-test/unit-test-slang-test-optimization-options.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| //CHECK1: unknown code generation target '-profile' | ||
|
|
||
| //DIAGNOSTIC_TEST:SIMPLE(diag=CHECK2):-profile ps_4_0 -target | ||
| //DIAGNOSTIC_TEST:SIMPLE(diag=CHECK2):-profile ps_4_0 -O3 -target |
There was a problem hiding this comment.
This doesn't make sense.
We shouldn't need this change.
There was a problem hiding this comment.
[Agent] Agreed — fixed at the root in 265ad27 and this test file is now unchanged from master. The real problem was that slang-test appended its default -O0 after the directive arguments, so a diagnostic test ending with an intentionally dangling -target would consume -O0 as its argument and report a different diagnostic. addDefaultSlangOptimization now inserts the default at the front of the argument list (slangc option parsing is position-independent), so test-provided arguments keep their meaning and no test needs to compensate. Validation: the slangTestDefaultOptimizationInsertion unit test now pins the front-insertion behavior with a dangling -target case, and tests/diagnostics/, tests/front-end/, and tests/cross-compile/ all pass locally (756/756).
| // which must be packed element-by-element into AnyValue storage. | ||
|
|
||
| //TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHECK):-dx12 -compute -shaderobj -output-using-type | ||
| //TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHECK):-dx12 -compute -shaderobj -output-using-type -Xslang -O2 |
There was a problem hiding this comment.
This should be -O3 not -O2
There was a problem hiding this comment.
[Agent] I tried -Xslang -O3 and it fails locally on dx12: dxc validation rejects the shader with 'Instructions should not read uninitialized value' at 'select i1 %15, float fadd (float fadd (float undef, float undef), float undef), ...' in computeMain, so the compute run returns no output and the CHECK comparison fails. At -O2 the test passes (5/5 locally, and it passed CI). So -O2 and -O3 do differ on the DXIL path: the Slang level maps to different downstream dxc optimization flags, and dxc at the higher level exposes an undef read in the generated HLSL. I kept -O2 for now. Two follow-ups for your call: (1) confirm keeping -O2 here, and (2) the undef read that dxc surfaces at -O3 may be a real codegen issue in the dynamic-dispatch/AnyValue path worth filing separately.
| @@ -1,5 +1,5 @@ | |||
| //TEST:SIMPLE(filecheck=CHECK_HLSL): -target hlsl -entry computeMain -stage compute | |||
| //TEST:SIMPLE(filecheck=CHECK_SPV): -target spirv-asm -entry computeMain -stage compute | |||
| //TEST:SIMPLE(filecheck=CHECK_SPV): -target spirv-asm -entry computeMain -stage compute -O1 | |||
There was a problem hiding this comment.
[Agent] I tried -O3 and the test fails: this SIMPLE test pins exact SPIR-V shapes (CHECK_SPV: OpAccessChain %_ptr_Function_v4float %m %int_3 followed by a second OpAccessChain and OpStore %float_11), and at -O3 spirv-opt's aggressive passes flatten those two-level access chains away, so the patterns cannot match. At -O1 the pinned shapes are present and the test passes — which is why the opt-in was -O1. I kept -O1. If you prefer -O3 here, the CHECK_SPV patterns would need to be rewritten against the optimized output, which weakens the element-level write-back assertions this regression test (#10212) exists to check. Please confirm which you prefer.
| ioCmdLine.addArg("-Xslang"); | ||
| ioCmdLine.addArg(isMetal ? kMetalRenderTestOptimizationOption : kTestOptimizationOption); |
There was a problem hiding this comment.
I think we should prepend the options rather than append to the end.
Because if the given test command line argument has a missing parameters by mistake, we don't want it to be mixed up with the newly appened options.
There was a problem hiding this comment.
[Agent] Done in 3e6ebdf. addDefaultRenderTestSlangOptimization now inserts -Xslang at the front of the argument list instead of appending, matching addDefaultSlangOptimization — so a directive that accidentally leaves an option without its parameter cannot consume the inserted default. render-test's parser collects positionals in order and stripDownstreamArgs handles forwarding pairs position-independently, so a leading -Xslang pair is safe. Validation: the three helper unit tests pass (including the updated dangling -xslang case, which now keeps the mistake visible as a trailing dangling flag), and pt-loop.slang, layout-array-2d.slang, and compute/simple.slang all pass locally through render-test (16/16). The README note now covers both command paths.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 0 bugs, 1 gap, 1 question
The PR is test-infrastructure only: no IR pass, emitter, or .meta.slang file is touched. The slang-test-optimization-options.h helpers (arg detection, front-insertion, Metal exception) are correctly implemented and thoroughly unit-tested. Slang's own IR pipeline is confirmed independent of the -O level; only spirv-opt gating and downstream-compiler flags change. The remaining concerns are about asymmetric per-line opt-ins in a few test files where only one backend received the new explicit level while sibling lines with the same CHECK contract dropped to -O0.
Changes Overview
slang-test default optimization (tools/slang-test/slang-test-optimization-options.h new, tools/slang-test/slang-test-main.cpp, tools/slang-test/README.md)
- New default
-O0for compiler-backed commands and-Xslang -O0for render-test-backed commands, front-inserted so trailing dangling options remain diagnosable. Metal render tests receive-Xslang -O1instead because the downstreammetaltoolchain is unstable at-O0on macOS CI. Detection recognizes slangc's canonical-O/-O0/-Onone/-O1/-Odefault/-O2/-Ohigh/-O3/-Omaximalspellings and the render-test forwarding forms-compile-arg,-xslang,-Xslang,-Xslang... -X..
Unit test coverage (tools/slang-unit-test/unit-test-slang-test-optimization-options.cpp new)
- Locks in the accepted
-Ospellings, all four forwarding forms, idempotent default insertion, front-insertion contract (verified with a dangling--targetshape), and Metal-mtl/-metalsynonym mapping to-O1.
Test opt-ins (~42 .slang/.hlsl files under tests/)
- Tests whose expected output or runtime behavior depends on
spirv-optor a downstream optimizer now declare an explicit-O<n>(mostly-O3, with-O2/-O1used where a higher level breaks the CHECK pattern or downstream validator).tests/autodiff/path-tracer/pt-loop.slangopts its-vkline into-Xslang -O1because-O0skipsspirv-optand the unoptimized SPIR-V crashed the Linux CI test server.
Findings (2 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | tests/language-feature/dynamic-dispatch/layout-array-2d.slang:5 |
Only -dx12 gets -Xslang -O2; -vk/-cuda/-cpu sharing the same CHECK buffer silently drop to -O0. Same pattern in layout-array-field.slang, layout-array-of-structs.slang, layout-array-of-vectors.slang. |
| 🔵 Question | tests/bugs/gh-9916.slang:2 |
Non-bindless line got -O3; CHECK_BINDLESS line did not — intentional, or overlooked? |
3 participants
Fixes #11804
Motivation
Release
slang-testruns spend substantial time compiling tests with optimization enabled even though most tests only need stable codegen or execution coverage. In the measured Release build, defaulting test compiler invocations to-O0reduced a full run from 34 min 9 sec to 11 min 24 sec.The SPIR-V tests also become less sensitive to
spirv-optoutput changes. When SPIR-V submodules are updated, optimizer differences can otherwise change expected output for tests that do not actually mean to exercise optimized SPIR-V. Skipping the optimizer by default makes those updates less noisy, while optimization-sensitive tests can still opt in explicitly.Proposed solution
slang-testnow appends a default-O0when it builds a Slang compiler command line and the test did not already provide a recognized optimization option. Compiler-backed paths receive-O0directly. Render-test-backed paths receive-Xslang -O0, which is the render-test spelling for forwarding the option to Slang.Every render-test-backed path receives an explicit slang-test-controlled default. Metal render tests receive
-Xslang -O1instead of-O0: the Slang optimization level is never read by Slang's own IR pipeline (the emitted MSL is byte-identical at every level), but it selects the flags passed to the downstreammetaltoolchain that produces the metallib, and macOS CI showed mass flaky failures (650+ tests, differing sets between jobs, fast empty-output failures) when that toolchain runs at-O0.-O1maps to the toolchain flags that were in effect before this PR, so Metal keeps the exact behavior that passes CI while still getting an explicit level like every other target.The override detection uses slangc's known optimization spellings instead of treating every
-O...option as an optimization level. That keeps unrelated future options from accidentally suppressing the slang-test default. Tests whose expected output or runtime behavior depends on optimization opt in explicitly with the lowest optimization level required by that backend.Change summary
tools/slang-test/slang-test-optimization-options.hcentralizes the default optimization option, direct compiler override detection, render-test forwarding detection, and render-test default insertion, including the Metal-specific-O1default (kMetalRenderTestOptimizationOption).tools/slang-test/slang-test-main.cppappends the shared default across compiler-backed and render-test-backed command builders, with compiler-backed paths adding it after_initSlangCompiler()for consistent duplicate-detection behavior.tools/slang-unit-test/unit-test-slang-test-optimization-options.cppcovers the accepted optimization spellings, render-test forwarding forms, idempotent default insertion, and the Metal-specific defaulting for-mtl/-metalspellings including the override case.tools/slang-test/README.mddocuments the new default, the compiler-backed opt-in spelling, the render-test-backed forwarding spellings (all four recognized forms), the Metal-O1exception, accepted optimization spellings, and the diagnostic-test pitfall.//TESTdirectives. This includestests/autodiff/path-tracer/pt-loop.slang, whose-vkdirective opts into-Xslang -O1because its unoptimized SPIR-V (spirv-opt skipped at-O0) crashed the Linux CI test server.Concepts and vocabulary
render-test, so Slang compiler arguments must be forwarded with render-test options such as-Xslang.-O0..-O3) does not change Slang's own IR passes; it controls whetherspirv-optruns (skipped entirely at-O0) and which flags downstream compilers (dxc, metal, nvrtc, gcc/clang) receive.Process report
The input shape reaching
slang-testis the parsed argument list from a//TESTdirective. That shape is intentional: tests can already specify compiler options, and an explicit optimization option is the right source of truth for optimization-sensitive tests. The helper only checks whether such an override exists before appending the default; it does not reinterpret the test directive or change how other options are parsed.For compiler-backed commands,
SlangTest::addDefaultSlangOptimizationruns after the command builder has added directive arguments (and after_initSlangCompiler()for paths that call it), so its override check sees the full argument list — but it inserts the default-O0at the front of the argument list rather than appending it. Front insertion makes the default position-independent: slangc option parsing accepts options in any order, while an appended-O0could be consumed as the argument of a trailing option a test intentionally leaves dangling. This is whytests/diagnostics/command-line/option-missing-argument.slangneeds no modification: its-profile ps_4_0 -targetdirective keeps-targetdangling and still reports the missing-argument diagnostic. TheslangTestDefaultOptimizationInsertionunit test pins the front-insertion contract with exactly that dangling--targetshape.For render-test-backed commands,
SlangTest::addDefaultRenderTestSlangOptimizationperforms the same check through render-test forwarding forms and inserts-Xslang <level>at the front of the argument list so the option reaches the Slang compiler rather than being treated as a render-test option. Front insertion here mirrors the compiler-backed path, per review: a directive that accidentally leaves an option without its required parameter cannot consume the inserted default, so the mistake stays visible instead of silently changing the command. This is safe because render-test's parser collects positional arguments in order andstripDownstreamArgshandles forwarding pairs position-independently. The helper recognizes-compile-arg,-xslang,-Xslang, and-Xslang... ... -X.forms. The unit test locks in those spellings and the front-insertion contract because a directive-level integration test could pass even if a helper accidentally inserted a stray-O0.Metal render tests receive
-O1instead of-O0. This was verified empirically across four CI rounds: commits without a Metal exception (968453a, 5b4c9df) failed 650+ Metal tests on macOS with flaky, fast, empty-output failures, while commits with one (ef2ec5f, eab84de) passed. The mechanism was traced in code: Slang's IR pipeline never reads the optimization level (verified by diffingslangc -target metaloutput at-O0vs default — byte-identical), so the only Metal-path difference isGCCDownstreamCompilerUtil::calcArgspassing-O0instead of-Osto themetalCLI when slang-rhi requestsSLANG_METAL_LIB. The instability therefore lives in the downstream Metal toolchain/runtime on the macOS runners, not in Slang, and is out of scope for this test-infrastructure PR. Choosing-O1(which maps back to-Os) keeps the uniform "slang-test always sets an explicit level" invariant while pinning Metal to the previously passing downstream flags. This supersedes the earlier fully-uniform-O0approach; see the Reviewer Directives section.tests/autodiff/path-tracer/pt-loop.slangopts its-vkdirective into-Xslang -O1. At-O0,spirv-optis skipped entirely (source/slang-glslang/slang-glslang.cpp,SLANG_OPTIMIZATION_LEVEL_NONEearly-out), and this heavy autodiff path-tracer's unoptimized SPIR-V consistently crashed the Linux CI test server (JSON RPC failure) on the NVIDIA driver while passing on Windows GPU.-O1restores exactly the pre-PR behavior for this one test, using the opt-in mechanism the issue prescribes.The remaining tests changed in this PR are the cases exposed by the
-O0default. Their expected output or runtime behavior depends on optimized Slang IR or downstream optimized codegen, so the directives now opt in explicitly. The opt-in levels are the lowest that keep each test passing, and they are not interchangeable:tests/language-feature/dynamic-dispatch/layout-array-2d.slangfails at-Xslang -O3because dxc's validator rejects the shader with an uninitialized-value (undef) read that only surfaces at the higher downstream optimization level, andtests/language-feature/swizzles/matrix-swizzle-out-arg-writeback.slangfails at-O3because spirv-opt's aggressive passes flatten the exactOpAccessChain/OpStoreshapes its FileCheck patterns pin, while-O1preserves them.Reviewer Directives (maintained by agent)
-O0demonstrably breaks 650+ Metal tests on macOS CI (see Process report). The current implementation keeps the uniform "always append an explicit level" shape but pins Metal to-O1(the previously passing downstream flags). Pending explicit confirmation from @jkwak-work..slangregression tests for this command-line default; keep the coverage focused in helper/unit tests instead. Source: Default slang-test compiler invocations to -O0 #11805 (comment)//TESTdirectives. (Stated during agent session, 2026-07-01.)-O3for optimization opt-ins where possible (requested forlayout-array-2d.slangandmatrix-swizzle-out-arg-writeback.slang). — Amended by validation evidence: both tests fail at-O3(dxcundef-read validation error on dx12; spirv-opt flattening the pinnedOpAccessChainshapes for spirv-asm), so they remain at-O2/-O1respectively; see the thread replies for the traces. This also supersedes the earlier session note that-O2and-O3are equivalent in practice — they map to different downstream dxc levels. Pending confirmation from @jkwak-work.