Allow SV_PrimitiveID in ray tracing hit stages#11423
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds SV_PrimitiveID support for ray-tracing hit stages by expanding the semantic stage list, validating and binding runtime system-value inputs, implementing IR legalization and backend plumbing (HLSL/GLSL/CUDA), mapping SPIR-V address spaces, updating emit wiring, and adding codegen and diagnostic tests. ChangesSV_PrimitiveID ray tracing hit-stage support
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba7eab78-0f00-444e-ad98-ede136284665
📒 Files selected for processing (11)
source/slang/core.meta.slangsource/slang/slang-emit.cppsource/slang/slang-ir-glsl-legalize.cppsource/slang/slang-ir-hlsl-legalize.cppsource/slang/slang-ir-hlsl-legalize.hsource/slang/slang-ir-spirv-legalize.cppsource/slang/slang-parameter-binding.cpptests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-ahit.slangtests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-chit.slangtests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-int.slangtests/hlsl-intrinsic/ray-tracing/sv-primitiveid-raytracing.slang
![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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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: dc284f9c-3c3f-4e6c-9851-02e3cd7b8f34
📒 Files selected for processing (3)
tests/diagnostics/execution-model/sv-primitiveid-rt-invalid.slangtests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-int.slangtests/hlsl-intrinsic/ray-tracing/sv-primitiveid-raytracing.slang
|
@coderabbitai review |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/slang/slang-ir-glsl-legalize.cpp (1)
3990-4009:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLegalize hit-stage
SV_PrimitiveIDbefore ray-tracing consolidation consumes its uses.Line 4935 still runs
consolidateRayTracingParameters()before the loop on Line 4945. In hit stages, that step can alreadyreplaceUsesWith()the originalSV_PrimitiveIDparameter: theoutParams.getCount() <= 1branch routes every varying param throughhandleSingleParam(), and theelsebranch still does it for non-outparams likeSV_PrimitiveID. By the time Line 4001 callstryLegalizeRayTracingPrimitiveIDParam(), the param often has no remaining uses, so the helper never materializes and this newprimitiveIndexFuncthreading staysnullptr.Example:
main_isect(int pid : SV_PrimitiveID)andmain_chit(..., int pid : SV_PrimitiveID)fromtests/hlsl-intrinsic/ray-tracing/sv-primitiveid-raytracing.slangcan reach the later legalization pass withpidalready rewritten to a generic global param. That skips the intended canonicalization to(gl_PrimitiveID)on the GLSL/SPIR-V-via-GLSL path.Suggested fix:
One safe direction
+ if (isRayTracingHitStage(stage)) + { + if (auto firstBlock = func->getFirstBlock()) + { + for (auto pp = firstBlock->getFirstParam(); pp; pp = pp->getNextParam()) + { + builder.setInsertBefore(firstBlock->getFirstOrdinaryInst()); + tryLegalizeRayTracingPrimitiveIDParam( + module, + builder, + pp, + primitiveIndexFunc, + /* removeParam */ false); + } + } + } + // Special handling for ray tracing shaders switch (stage) { case Stage::AnyHit: case Stage::Callable:The alternative is to make
consolidateRayTracingParameters()explicitly skipSV_PrimitiveID, but the key is that the helper has to see live uses before consolidation rewrites the parameter.Also applies to: 4960-4966, 5092-5098
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 293e4fd6-e362-4e4f-950a-105348607095
📒 Files selected for processing (3)
source/slang/slang-ir-glsl-legalize.cppsource/slang/slang-ir-spirv-legalize.cppsource/slang/slang-parameter-binding.cpp
|
[Agent] Addressed CodeRabbit's note about GLSL hit-stage The GLSL path now attempts Validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -4909,6 +4912,26 @@ void legalizeEntryPointForGLSL( | |||
| invokePathConstantFuncInHullShader(&context, codeGenContext, scalarizedGlobalOutput); | |||
| } | |||
|
|
|||
| if (isRayTracingHitStage(stage) && | |||
| !codeGenContext->getTargetProgram()->shouldEmitSPIRVDirectly()) | |||
There was a problem hiding this comment.
Because this file is about GLSL legalization, the following condition is assumed to be true all the time,
!codeGenContext->getTargetProgram()->shouldEmitSPIRVDirectly())
It is assumed that this check happened at an earlier stage before come this the GLSL legalization or at the early stage.
If needed, it should be SLANG_ASSERT but it shouldn't be needed.
There was a problem hiding this comment.
[Agent] I tested removing the guard and did not keep that change. Direct SPIR-V ray-tracing tests do enter this legalizer with shouldEmitSPIRVDirectly() true; removing the guard materialized the GLSL PrimitiveIndex helper in direct SPIR-V without the required import linkage and caused six focused SPIR-V checks to fail with Function declaration must have a LinkageAttributes decoration. The guard remains so direct SPIR-V stays on its BuiltIn PrimitiveId path. Validation after restoring the guard and committing e12ab61: cmake.exe --build --preset debug --target slangc slang-test; focused slang-test.exe set passed 43/43; git.exe diff --check.
There was a problem hiding this comment.
In that case, we should run the check only once at an early stage and store somewhere rather than call the function all the time.
The current implementation gives us the impression that the implementation expect the value to change more frequently than it does.
There was a problem hiding this comment.
[Agent] Addressed in 4f4b6c9. legalizeEntryPointForGLSL() now computes shouldEmitSPIRVDirectly once immediately after reading the entry-point stage and reuses that stored value for the ray-tracing hit-stage SV_PrimitiveID legalization gate. This keeps the direct-SPIR-V decision fixed at the early entry-point legalization point instead of calling through TargetProgram at the use site.
Validation: build/Debug/bin/slang-test.exe tests/hlsl-intrinsic/ray-tracing/sv-primitiveid-raytracing tests/cuda/sv-primitiveid-raytracing tests/diagnostics/execution-model/sv-primitiveid-rt-invalid tests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-ahit tests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-chit tests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-int passed 47/47; git.exe diff --check reported only the expected LF-to-CRLF warning.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jkwak-work
left a comment
jkwak-work
left a comment
There was a problem hiding this comment.
Please reduce/remove unnecessary changes and minimize the code changes as much as you can
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| Session* session, | ||
| IRModule* module, | ||
| IRFunc* func, | ||
| CodeGenContext* codeGenContext, | ||
| ShaderExtensionTracker* glslExtensionTracker) | ||
| ShaderExtensionTracker* glslExtensionTracker, | ||
| IRFunc* primitiveIndexFunc) |
There was a problem hiding this comment.
Remind me why this function has to take primitiveIndexFunc?
Can we keep the original function signature?
There was a problem hiding this comment.
[Agent] Addressed in 5190000. legalizeEntryPointForGLSL() now keeps its original signature; the primitiveIndexFunc cache is local to the hit-stage parameter legalization block. The previous argument was only carrying the helper across GLSL entry points, but this use only needs a per-entry-point cache while walking that entry point's params before ray-tracing consolidation.
Validation: cmake.exe --build --preset debug --target slangc slang-test; focused build/Debug/bin/slang-test.exe tests/hlsl-intrinsic/ray-tracing/sv-primitiveid-raytracing tests/cuda/sv-primitiveid-raytracing tests/diagnostics/execution-model/sv-primitiveid-rt-invalid tests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-ahit tests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-chit tests/hlsl-intrinsic/ray-tracing/rt-pipeline-intrinsics-int passed 47/47; git.exe diff --check reported only the expected LF-to-CRLF warning.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Verdict: ✅ Clean — no significant issues found
PR enables SV_PrimitiveID as an input semantic in ray-tracing intersection/anyhit/closesthit stages and routes it to per-target intrinsics (PrimitiveIndex() for HLSL/DXIL, (gl_PrimitiveID) for GLSL via SPIR-V, optixGetPrimitiveIndex() for CUDA/OptiX, BuiltIn PrimitiveId for direct SPIR-V). Targets without ray tracing (Metal, WGSL) emit an Unimplemented diagnostic; raygen/miss/callable stages and out/inout directions are rejected by parameter binding and semantic-check rules respectively. IR-pass review confirmed iterator safety on the GLSL param loop (next captured before removeAndDeallocate), traverseUses snapshot-iteration safety in replacePrimitiveIDParamUses, correct result == AddressSpace::Generic precondition in the new SPIR-V legalize branch, and that the two-pass restructure of EntryPointVaryingParamLegalizeContext::processModule preserves behavior for existing CUDA/CPU subtypes (shouldProcessEntryPoint defaults to true). Test coverage exercises all four backend paths plus negative cases for stage, direction, target, and type.
Changes Overview
Semantic / parameter-binding allowlist (source/slang/core.meta.slang, source/slang/slang-parameter-binding.cpp)
core.meta.slang: adds[require(intersection)],[require(anyhit)],[require(closesthit)]to thegetaccessor ofsv_primitiveid.slang-parameter-binding.cpp: allowlistsSV_PrimitiveIDas a runtime-provided varying input in hit stages for D3D / Khronos / CUDA targets, falling through to ordinary varying-input handling instead of the hit-attributes layout. Unsupported targets (Metal, WGSL) get anUnimplementeddiagnostic.
IR legalization helpers (source/slang/slang-ir-legalize-varying-params.cpp/.h)
- New
tryLegalizeRayTracingPrimitiveIDParamhelper rewrites SV_PrimitiveID params into a call to a synthesizedIRFunccarrying per-targettargetIntrinsicdecorations for HLSL/GLSL/CUDA. - New
HLSLRayTracingPrimitiveIDParamLegalizeContextreuses the helper inside theEntryPointVaryingParamLegalizeContextframework; that framework is restructured to a two-pass collect-then-process model with a newshouldProcessEntryPointfilter hook. - CUDA path adds
case SystemValueSemanticName::PrimitiveIDthat routes through the same value emitter.
Backend wiring (source/slang/slang-emit.cpp, source/slang/slang-ir-hlsl-legalize.cpp/.h, source/slang/slang-ir-glsl-legalize.cpp, source/slang/slang-ir-spirv-legalize.cpp)
slang-emit.cpp/slang-ir-hlsl-legalize.{cpp,h}: pass renamedlegalizeNonStructParameterToStructForHLSL→legalizeParametersForHLSL; new dispatcher also callslegalizeRayTracingPrimitiveIDParamsForHLSL.slang-ir-glsl-legalize.cpp:legalizeEntryPointForGLSLinvokestryLegalizeRayTracingPrimitiveIDParamover hit-stage params when!shouldEmitSPIRVDirectly()(so SPIR-V via GLSL routes throughgl_PrimitiveID).slang-ir-spirv-legalize.cpp: in direct SPIR-V path, setsAddressSpace::BuiltinInputforsv_primitiveidparameters in hit stages when no varying-input offset attr was assigned (result == AddressSpace::Generic).
Tests (tests/cuda/sv-primitiveid-raytracing.slang, tests/diagnostics/execution-model/sv-primitiveid-rt-invalid.slang, tests/hlsl-intrinsic/ray-tracing/{rt-pipeline-intrinsics-ahit,rt-pipeline-intrinsics-chit,rt-pipeline-intrinsics-int,sv-primitiveid-raytracing}.slang)
- CUDA: filechecks for
optixGetPrimitiveIndex()and absence ofprimitiveIDparameter in__closesthit__entry point, including int→uint conversion. - Diagnostics: negative tests for raygen/miss/callable (rejected stages), out/inout (rejected directions), Metal/WGSL (unsupported targets), and struct-typed semantic (BAD_TYPE).
- Ray-tracing intrinsics suite: per-stage SPIR-V (
OpDecorate ... BuiltIn PrimitiveId), DXIL (@dx.op.primitiveIndex.i32), GLSL (gl_PrimitiveID), CUDA (optixGetPrimitiveIndex()), unused-param case, multi-entry-point case, and a forward-call case (helper function called from a hit shader).
2 participants
Fixes #11197
Summary of the problem from the end user perspective
SV_PrimitiveIDcould not be used consistently as an input in ray tracing intersection, any-hit, and closest-hit shaders. Shaders that need the primitive index in those stages had to use backend-specific forms instead of the semantic spelling.Minimal repro shader; if applicable
Root cause
The semantic metadata and parameter binding rules did not allow
SV_PrimitiveIDin ray tracing hit stages. Backend legalization also needed target-specific handling so the value is not treated like payload or hit-attribute data.Solution in this PR
The PR allows
SV_PrimitiveIDin intersection, any-hit, and closest-hit stages. SPIR-V lowers it asBuiltIn PrimitiveIdinput, SPIR-V-via-GLSL routes throughgl_PrimitiveID, HLSL/DXIL emission replaces uses with the nativePrimitiveIndex()intrinsic, and CUDA/OptiX emission replaces uses withoptixGetPrimitiveIndex().Notes to the reviewers; where to focus on
The main review points are the semantic/stage allowlist changes, the parameter-binding behavior for ray tracing system-value inputs, and the SPIR-V, GLSL, HLSL, and CUDA legalization paths. Direct SPIR-V ray tracing entry points still reach the GLSL legalization pass, so the
shouldEmitSPIRVDirectly()guard in that pass is intentional.Reviewer Directives (maintained by agent)
SV_PrimitiveIDSPIR-V test directives unqualified by explicit-emit-spirv-directlyor-emit-spirv-via-glsl; the CI/test matrix covers both SPIR-V emission paths. (Allow SV_PrimitiveID in ray tracing hit stages jkwak-work/slang#218 (comment))Related PRs in the past