Fix #11197: accept SV_PrimitiveID as input in intersection, any-hit, and closest-hit shaders#11894
Draft
jkwak-work wants to merge 1 commit into
Draft
Fix #11197: accept SV_PrimitiveID as input in intersection, any-hit, and closest-hit shaders#11894jkwak-work wants to merge 1 commit into
jkwak-work wants to merge 1 commit into
Conversation
…and closest-hit shaders
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11197
Motivation
GLSL_EXT_ray_tracing allows
gl_PrimitiveIDas an input in the intersection, any-hit, and closest-hit stages, but Slang rejects the equivalentSV_PrimitiveIDinput. Consider this example (the reproducer from #11197):Compiling this to SPIR-V fails with:
The only workaround is the
PrimitiveIndex()intrinsic, which maps to the same value but is hard to discover for users porting GLSL ray-tracing shaders that readgl_PrimitiveID.Proposed solution
Fixing the reported error exposed two more issues cascading behind it; each is fixed at the layer that produces the wrong representation, so no consumer needs to patch around it:
Front-end validation (the reported E30702): the
sv_primitiveidsemantic declaration incore.meta.slangis the source of truth for which stages may read a system value. Itsgetaccessor only listed the rasterization stages, so the fix is to extend it with[require(intersection)],[require(anyhit)], and[require(closesthit)]— exactly the three stages where GLSL_EXT_ray_tracing permitsgl_PrimitiveID. Stages with no current primitive (raygeneration, miss, callable) remain rejected.Parameter binding (cascading E39018): entry-point layout interpreted every ray-tracing entry-point parameter as a payload/hit-attributes parameter by direction and position, before the system-value classification could run. The fix teaches the ray-tracing interpretation that a parameter carrying an ordinary system-value semantic is not a ray-tracing-role parameter; it takes the regular system-value layout path that every other stage already uses.
GLSL/SPIR-V legalization (invalid SPIR-V): the ray-tracing entry-point legalization turned every varying parameter into a plain global parameter (correct for payload/attributes, wrong for system values — it produced a
Private-storage variable decoratedBuiltIn PrimitiveId, which fails SPIR-V validation). The fix routes system-value parameters through the standard varying-input legalization, which maps them to a properInput-class builtin — the same machinery a fragment-shaderSV_PrimitiveIDinput uses.With all three fixes, the downstream SPIR-V emitter needs no changes:
maybeEmitSystemVal(slang-emit-spirv.cpp:7624) already mapssv_primitiveidtoBuiltIn PrimitiveIdand already exempts the intersection/any-hit/closest-hit stages from theGeometrycapability requirement, so the emit side had anticipated this input all along.Change summary
source/slang/core.meta.slang[require(intersection)],[require(anyhit)],[require(closesthit)]to theget : uintaccessor ofsemantic sv_primitiveid.source/slang/slang-parameter-binding.cppisRayTracingPayloadOrAttributesParametergates the ray-tracing payload/attributes interpretation inprocessEntryPointVaryingParameter; extractedisSystemValueSemanticNameso thesv_/nv_prefix rule has one source of truth (also used byprocessSimpleEntryPointParameter).source/slang/slang-ir-glsl-legalize.cppconsolidateRayTracingParametersskips parameters whoseIRVarLayoutcarries a system-value semantic; the ray-tracing early-return inlegalizeEntryPointParameterForGLSLlets those parameters continue to the standard varying-input path.tests/vkray/intersection-primitive-id.slangSV_PrimitiveID; checks SPIR-VBuiltIn PrimitiveIdon anInput-storage-class variable andgl_PrimitiveIDin GLSL output.tests/diagnostics/execution-model/sv-primitive-id-invalid-rt-stage.slangSV_PrimitiveIDinput in a miss shader still produces E30702.Concepts and vocabulary
core.meta.slangassemantic sv_*declarations whoseget/setaccessors carry[require(stage)]attributes;validateSystemValueSemanticForType(slang-check-shader.cpp) walks these accessors to decide whether a semantic is readable/writable in a given stage. This table — not the validation code — owns the per-stage policy.inout/outparameter is the ray payload (or callable payload), aninparameter of a hit shader is the hit attributes.SV_RayPayloadandSV_IntersectionAttributesare SV-prefixed semantics that explicitly mark these same roles (seetests/spirv/c-layout-buffer-2.slang).processSimpleEntryPointParametergives a system-value parameter a layout that consumes no varying location slots and records the semantic invarLayout->systemValueSemantic, which lowers toIRSystemValueSemanticAttron the parameter'sIRVarLayout.Process report
Change 1 —
core.meta.slangaccessor stages (the reported error).validateSystemValueSemanticForType(slang-check-shader.cpp:116) looks upsemantic sv_primitiveidand walks its accessors; for eachgetaccessor it checks the merged[require(...)]capability set against the entry point's stage viacapabilitySet->isIncompatibleWith(getAtomFromStage(stage)). The accessor listed onlyfragment,geometry,hull, anddomain, so for an intersection shader no accessor matched and the E30702 diagnostic fired. Theintersection/anyhit/closesthitcapability aliases (slang-capabilities.capdef:1476-1484) expand to the_intersection/_anyhit/_closesthitstage atoms joined with theraytracingtargets, so adding those three[require(...)]attributes makes the stage check pass exactly where the GLSL spec allowsgl_PrimitiveID. The reproducer declaresintwhile the accessor declaresuint;isSemanticTypeCompatible(slang-check-shader.cpp:78) accepts this becauseintcoerces touintwith matching scalar shape. No new stage is opened for writing (setremains geometry/mesh), and a new diagnostic test pins that miss/raygeneration inputs still fail.Change 2 — parameter binding (cascading E39018).
With change 1 alone, the reproducer fails with
error[E39018]: the 'intersection' stage does not support 'in' entry point parameters. The producer isprocessEntryPointVaryingParameter(slang-parameter-binding.cpp): for ray-tracing stages it classifies parameters by direction — input in a hit shader becomes hit attributes, input in intersection/raygeneration/miss/callable is diagnosed, output becomes payload — and this runs beforeprocessSimpleEntryPointParameterever sees theSV_prefix.Input-shape check: is "an entry-point parameter with a system-value semantic in a ray-tracing stage" a shape this branch should claim? No. The ray-tracing interpretation exists to assign the payload/attributes roles, which are marked either implicitly (no semantic, or a user semantic, resolved by direction and position) or explicitly by
SV_RayPayload/SV_IntersectionAttributes. A parameter carrying any other system-value semantic is a builtin input, and the per-stage policy for builtins is already owned by the accessor table from change 1. So the correct producer-side fix is for the layout code to leave such parameters to the regular system-value path — not for any downstream consumer to undo a payload/attributes misclassification.The new
isRayTracingPayloadOrAttributesParameterencodes exactly that rule and gates the existing if/else. Behavior is unchanged for every previously-accepted program: parameters without semantics or with user semantics keep the ray-tracing path (e.g.tests/diagnostics/execution-model/stage-incompatible-out-param.slang, whereout float3 x : MY_SEMANTICin an intersection shader still produces E39017), andSV_RayPayload/SV_IntersectionAttributeskeep it too (e.g.tests/spirv/c-layout-buffer-2.slang). Non-ray-tracing stages are unaffected because both gated switches were no-ops for them. Thesv_/nv_prefix test previously appeared inline inprocessSimpleEntryPointParameter; it is extracted intoisSystemValueSemanticNameso the classification rule lives in one place.Change 3 — GLSL/SPIR-V legalization (invalid SPIR-V).
With changes 1 and 2, compilation succeeds but SPIR-V validation fails:
Vulkan spec allows BuiltIn PrimitiveId to be only used for variables with Input or Output storage class ... uses storage class Private, and the GLSL target emits a bareuint primitiveId_0;global instead ofgl_PrimitiveID. The producer is the ray-tracing branch of the GLSL legalization pass:consolidateRayTracingParameters(slang-ir-glsl-legalize.cpp:3118) claims every varying parameter of a ray-tracing entry point and hands it tohandleSingleParam, which creates a plain global parameter — correct for payload/attributes (their storage classes come from their own decorations) but wrong for a system value, which needs the builtin mapping. MeanwhilelegalizeEntryPointParameterForGLSLearly-returns for ray-tracing stages, so the standard varying path —createGLSLGlobalVaryings, whosegetGLSLSystemValueInfo(slang-ir-glsl-legalize.cpp:730) mapssv_primitiveidtogl_PrimitiveIDwith required typeintand anInput-class builtin variable — never runs.Input-shape check: same answer as change 2, applied at the IR layer. The canonical representation of a system-value input is the varying layout with
IRSystemValueSemanticAttrproduced by parameter binding; the ray-tracing consolidation should not rewrite it into a plain global. Both edits consult that existing attribute (paramLayout->findSystemValueSemanticAttr()) rather than re-deriving anything: the consolidation skips such parameters, and the early-return lets them continue into the same default varying-input path a fragment-shaderSV_PrimitiveIDinput takes. After this, the SPIR-V emitter's existingmaybeEmitSystemValhandling produces%gl_PrimitiveID = OpVariable %_ptr_Input_int InputdecoratedBuiltIn PrimitiveId, and validation passes; the GLSL target emitsgl_PrimitiveIDwith the int→uint adaptation.Target notes.
ReportHit(E36107) independent of this change;PrimitiveIndex()→optixGetPrimitiveIndexremains the OptiX route.SV_PointCoordandSV_DrawIndexon D3D targets. Legalizing ray-tracing system-value parameters intoPrimitiveIndex()calls on D3D would require a varying-parameter legalization pass for HLSL, which does not exist today; left as a follow-up.Validation. Full
slang-testrun: 11150/11151 passed; the one failure (tests/dispatcher/smoke.slang) was a local build-scope artifact (theslangdispatcher binary had not been built) and passes after building it.