Don't warn E31106/E31107 on compiler-synthesized parameter groups#11986
Conversation
Entry-point uniform/resource parameters (and global-scope shader parameters) are gathered by the compiler into a synthesized ConstantBuffer<EntryPointParams>/<GlobalParams>. When a resource then leaks out of that buffer during type legalization we no longer emit the "special type leaks from parameter group" warnings: the user wrote a flat parameter/global list, not the grouping, so there is nothing to restructure (and the warnings had no source location). Mark the synthesized element struct with a new SynthesizedParameterGroupDecoration and skip the diagnostic for it. The marker is preserved across type legalization (which moves the original struct's decorations onto a new ordinary struct), so the buffer is still recognized as synthesized when it is re-legalized. User-authored ConstantBuffer<S> groups that mix resources and ordinary data still warn, with a location. Fixes shader-slang#11825
📝 WalkthroughWalkthroughChangesThe compiler now marks synthesized parameter-group structs with a dedicated IR decoration, preserves that marker through type legalization, suppresses corresponding leak diagnostics, updates IR metadata, and adds global and entry-point regression tests. Synthesized parameter group decoration
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 |
There was a problem hiding this comment.
Pull request overview
This PR suppresses E31106/E31107 “special type leaks from parameter group” warnings when the offending parameter group was compiler-synthesized (entry-point uniform/resource collection and global-uniform collection), avoiding confusing, locationless diagnostics that users cannot act on.
Changes:
- Introduces
SynthesizedParameterGroupDecorationto tag compiler-synthesized parameter-group element structs. - Applies the decoration when synthesizing
EntryPointParamsandGlobalParams, and suppresses the leak warning in type legalization when the marker is present (while preserving it acrosstransferDecorationsTo). - Adds a diagnostics regression test ensuring the entry-point synthesized case is silent for HLSL and SPIR-V.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/diagnostics/entry-point-uniform-no-parameter-group-leak-warning.slang | Regression test asserting no diagnostics for the synthesized entry-point parameter group case on HLSL/SPIR-V. |
| source/slang/slang-legalize-types.cpp | Suppresses E31106/E31107 for marked synthesized groups and preserves the marker across decoration transfer during struct legalization. |
| source/slang/slang-ir-insts.lua | Adds the IR decoration opcode definition for SynthesizedParameterGroupDecoration. |
| source/slang/slang-ir-insts.h | Declares the new decoration type and adds an IRBuilder helper to attach it. |
| source/slang/slang-ir-insts-stable-names.lua | Adds a stable-name entry for the new decoration. |
| source/slang/slang-ir-entry-point-uniforms.cpp | Marks the synthesized EntryPointParams struct as a synthesized parameter group. |
| source/slang/slang-ir-collect-global-uniforms.cpp | Marks the synthesized GlobalParams wrapper struct as a synthesized parameter group. |
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: f97deb36-c1d0-46e7-b257-d986bb3c11fc
📒 Files selected for processing (7)
source/slang/slang-ir-collect-global-uniforms.cppsource/slang/slang-ir-entry-point-uniforms.cppsource/slang/slang-ir-insts-stable-names.luasource/slang/slang-ir-insts.hsource/slang/slang-ir-insts.luasource/slang/slang-legalize-types.cpptests/diagnostics/entry-point-uniform-no-parameter-group-leak-warning.slang
|
This PR modifies IR instruction definition files. Please review if you need to update the following constants in
These version numbers help ensure compatibility between different versions of compiled modules. |
- Bump k_maxSupportedModuleVersion for the new IR op so older compilers reject modules that use it. - Mark the OptiX shader-record (SBT) synthesized param struct too, so ray-tracing entry points don't hit the same locationless warning. - Add SynthesizedParameterGroupDecoration to isSimpleDecoration so re-attaching the marker deduplicates instead of accumulating. - Fold the marker preservation into copyNameHintAndDebugDecorations rather than a one-off re-add, keeping one source of truth for decorations that must survive transferDecorationsTo. - Add a global-scope regression test covering the GlobalParams synthesis path.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/slang/slang-legalize-types.cpp (1)
1246-1256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale rationale: the marker is restored onto
originalElementTypeafterlegalizeType.The comment states Whether this parameter group was synthesized by the compiler rather than written by the user. We check for the marker before legalizing the element type below: legalizing a struct with mixed resource/ordinary fields creates a new "ordinary" struct and calls
transferDecorationsToto move the original struct's decorations (including this marker) onto it, so afterlegalizeTypethe marker is no longer onoriginalElementType.However,
TupleTypeBuilder::getResult()(this same file) calls originalStructType->transferDecorationsTo(ordinaryStructType); ... copyNameHintAndDebugDecorations(originalStructType, ordinaryStructType); right after the transfer, andcopyNameHintAndDebugDecorationsexplicitly clonesSynthesizedParameterGroupDecorationback ontodest(originalStructType==originalElementType). So by the timelegalizeType(context, originalElementType)returns, the marker is back onoriginalElementType, contradicting the comment's claim. Checking before the call is still harmless, but the stated justification is inaccurate and could mislead a future reader reasoning about this invariant — especially since this exact preservation mechanism was already the subject of prior review discussion.Consider rewording to something like "we capture the marker before the call for simplicity/clarity, though it is in fact restored onto
originalElementTypeafterward viacopyNameHintAndDebugDecorations."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fb43173-f274-4602-8d05-4f0ae9a88571
📒 Files selected for processing (6)
source/slang/slang-ir-optix-entry-point-uniforms.cppsource/slang/slang-ir-util.cppsource/slang/slang-ir.cppsource/slang/slang-ir.hsource/slang/slang-legalize-types.cpptests/diagnostics/global-uniform-no-parameter-group-leak-warning.slang
The OptiX shader-record synthesis site is not reached by the E31106/E31107 leak diagnostic: CUDA/OptiX legalizes resource parameters without letting them leak as a pair type out of the ConstantBuffer, so the diagnose site is never hit (verified with a breakpoint on a raygeneration entry point with mixed resource + uniform parameters). Since no test fails without the marking there, remove it rather than keep speculative defensive code. Also correct the rationale comment at the buffer-legalization read site: the marker is now restored onto the original struct by copyNameHintAndDebugDecorations, so explain that we read it before legalizeType to stay independent of that restore rather than because it is gone afterwards.
Revert the addition of SynthesizedParameterGroupDecoration to the shared copyNameHintAndDebugDecorations helper: that helper is also used to flatten varying-IO structs and to create debug variables, where the marker has no meaning, so cloning it there makes the marker's scope ambient. Instead re-add the marker on the original struct at the parameter-group struct-splitting site in legalizeTypeImpl, keeping its meaning narrow to parameter-group structs.
There was a problem hiding this comment.
Verdict: ✅ Clean — no significant issues found
Adds an IRSynthesizedParameterGroupDecoration marker to compiler-synthesized parameter-group element structs (EntryPointParams, GlobalParams) and skips the E31106/E31107 "special type leaks from parameter group" diagnostic for them in legalizeTypeImpl, while leaving user-authored ConstantBuffer<S> groups warning as before. Four reviewer passes (code-quality, IR-correctness, security, test-coverage) plus a clarity pass surfaced no findings that survive verification.
Changes Overview
New IR op (slang-ir-insts.lua, slang-ir-insts-stable-names.lua, slang-ir-insts.h, slang-ir.cpp, slang-ir.h)
- Adds the operand-less
SynthesizedParameterGroupDecoration(stable name 891, next after 890), anaddSynthesizedParameterGroupDecorationbuilder helper, registers it inisSimpleDecorationso re-attaching dedups, and bumpsk_maxSupportedModuleVersion24→25. Stable-name serialization means older compilers map the unknown op tokIROp_Unrecognized(harmless: a suppression is simply not applied).
Marking the synthesis sites (slang-ir-entry-point-uniforms.cpp, slang-ir-collect-global-uniforms.cpp)
- Marks the synthesized element struct at construction — the correct producer layer.
Diagnostic suppression + marker preservation (slang-legalize-types.cpp)
- Reads
isSynthesizedGroupoff the element struct before legalizing it and adds&& !isSynthesizedGroupto the leak-warning guard; re-adds the marker onto the original struct aftertransferDecorationsTomoves it, so the group is still recognized on later legalization passes (each pass builds a fresh legalization context).
Regression tests (tests/diagnostics/entry-point-uniform-...slang, global-uniform-...slang)
- Exhaustive
DIAGNOSTIC_TESTon SPIRV + HLSL asserting both synthesis sites compile silently; the pre-existingparameter-group-special-type-leak-location*.slangremain as the positive control that user-authored groups still warn.
reviewed: ad3ee98 · diff sha256 f97d3512b558
258a984
Motivation
Compiling this shader with
-target hlslor-target spirvproduces three warnings with no source location:The user wrote a flat parameter list, not a parameter group. The compiler gathers the entry-point
uniform/resource parameters into a synthesizedConstantBuffer<EntryPointParams>, and when the textures then "leak" out of that constant buffer during type legalization we warn — but that regrouping is inherent to how we lower entry-point parameters, so there is nothing for the user to restructure. The warnings are also locationless (the synthesized type has no source loc), which is exactly the confusing case #11825 reports.The warning is correct and useful for user-authored mixed groups (its original purpose, PR #10158), so we only suppress it for the compiler-synthesized case.
Proposed solution
Mark the compiler-synthesized parameter-group element struct with a new
SynthesizedParameterGroupDecorationat the places we synthesize such a struct (entry-point uniform collection and global-uniform collection), and skip the E31106/E31107 diagnostic inlegalizeTypeImplwhen the constant buffer's element carries that marker. User-authoredConstantBuffer<S>groups are unmarked and continue to warn, with a location.Change summary
slang-ir-insts.lua,slang-ir-insts-stable-names.lua,slang-ir-insts.hSynthesizedParameterGroupDecorationIR op (+ stable name +addSynthesizedParameterGroupDecorationbuilder helper).slang-ir.hk_maxSupportedModuleVersion24 → 25 for the new IR op.slang-ir.cppisSimpleDecorationso re-attaching it deduplicates.slang-ir-entry-point-uniforms.cppEntryPointParamsstruct.slang-ir-collect-global-uniforms.cppGlobalParamsstruct.slang-legalize-types.cpptransferDecorationsToand later legalization passes.tests/diagnostics/entry-point-uniform-no-parameter-group-leak-warning.slang,.../global-uniform-...slangConcepts and vocabulary
ConstantBuffer<EntryPointParams>/ConstantBuffer<GlobalParams>the compiler builds to hold entry-pointuniform/resource parameters or global-scope shader parameters. The user never writes this grouping.transferDecorationsTo— moves all decorations off one inst onto another. Struct legalization uses it to hand the original struct's decorations to the new "ordinary" struct it builds for the non-resource fields, leaving the original as a husk.LegalType— the result of legalizing a struct that mixes resource and ordinary fields: an "ordinary" part plus a "special" (resource) part. A pair element inside aConstantBufferis exactly the "resource leaks out of the group" condition the warning fires on.Process report
Marking the synthesized structs. The two producers that synthesize a parameter-group element struct and are reachable by the leak diagnostic are
CollectEntryPointUniformParams::ensureCollectedParamAndTypeHaveBeenCreated(EntryPointParams) andcollectGlobalUniformParameters(GlobalParams). Marking the struct at construction is the right layer — the marker is a true property of these structs, not a downstream repair. This answers the input-shape check: the "constant buffer whose element leaks a resource" shape reaching the diagnostic is valid for user code, so we don't change the diagnostic's contract; we only distinguish the synthesized producer, at the producer. (The analogous OptiX shader-record synthesis site was investigated too, but the leak diagnostic is not reachable there — CUDA/OptiX legalizes resource params without a pair-leak out of the constant buffer, verified with a breakpoint on the diagnose site — so no marking is added there, since nothing would test it.)Skipping the diagnostic. In
legalizeTypeImplthe leak warning fires whenlegalElementType.flavor == pair && as<IRConstantBufferType>(type). We add&& !isSynthesizedGroup, read from the element struct's marker before the recursivelegalizeTypecall.Preserving the marker across legalization (the cascading part). The first attempt (just checking the marker at the buffer site) still warned. Tracing it: the buffer
ConstantBuffer<EntryPointParams>is legalized, and legalizing its element struct hits the mixed-resource/ordinary path that builds a newordinaryStructTypeand callsoriginalStructType->transferDecorationsTo(ordinaryStructType)— which moves the marker off the original struct. The buffer is then legalized a second time on the same pointer (resource legalization runs as more than one pass over the module), and on that pass the element struct no longer carries the marker, so the warning fired. Debug tracing confirmed the samebuf/elempointers legalized twice: firstsynth=1 flavor=simple(no warning), thensynth=0 flavor=pair(warning).The fix re-adds the marker onto the original struct at the struct-splitting site in
legalizeTypeImpl(right aftertransferDecorationsTo), so the buffer is still recognized as synthesized on subsequent passes. The preservation is deliberately kept site-local rather than folded into the sharedcopyNameHintAndDebugDecorationshelper: that helper is also used to flatten varying-IO structs and to create debug variables, where the "synthesized parameter group" marker has no meaning, so cloning it there would make the marker's scope ambient. Keeping it at the parameter-group struct-splitting site keeps the marker's meaning narrow. Combined with adding the op toisSimpleDecoration, re-adding the marker deduplicates rather than accumulating.Confirmed behavior after the fix: the issue repro compiles with zero E31106/E31107 on both HLSL and SPIRV; the global-scope repro (
global-uniform-...slang) likewise; a userConstantBuffer<S>mixing a resource and ordinary data still emits both warnings with a location (existingtests/diagnostics/parameter-group-special-type-leak-location*.slangcontinue to pass).