Fix abort extension name#11716
Conversation
…r_abort The SPIRV grammar (spirv.core.grammar.json) registers the AbortKHR capability under the extension name "SPV_KHR_abort", but the emitter was declaring "SPV_KHR_shader_abort". The validator's IsEnabledByExtension check looks up the capability's associated extensions from the grammar table, so the mismatch caused validation to fail even though the extension string was present. Fix the emitted extension string and update the test to remove the now-unnecessary -skip-spirv-validation workaround and stale comment.
Now that the emitter uses the correct extension name SPV_KHR_abort (matching the SPIRV grammar), spirv-val passes without skipping validation.
|
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:
📝 WalkthroughWalkthroughThe PR corrects the SPIR-V extension name from ChangesSPV_KHR_abort Extension Name Fix and Validation Re-enablement
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 1 question
PR renames the SPIR-V extension from the non-registry SPV_KHR_shader_abort to the canonical SPV_KHR_abort, splits out a spvAbort capability atom for AbortKHR, regenerates docs, and removes -skip-spirv-validation from abort tests. The bundled spirv-tools (external/spirv-tools-generated/core_tables_header.inc:154: kSPV_KHR_abort,) recognizes the canonical name, so dropping the validation skip is supported. Five VK_KHR_shader_abort references in adjacent doc comments are not updated — flagged for author intent.
Changes Overview
SPIR-V extension rename (source/slang/slang-emit-spirv.cpp, source/slang/slang-capabilities.capdef)
emitAbortnow emitsOpExtension "SPV_KHR_abort"(the Khronos registry-canonical name) instead of the non-registrySPV_KHR_shader_abort.- The capdef extension atom is renamed to
SPV_KHR_abort. A newspvAbortcapability atom forAbortKHRis added, andGL_EXT_shader_abortnow ORs_GL_EXT_shader_abort | spvAbortinstead of... | SPV_KHR_shader_abort. The user-facingabortcapability alias is unchanged.
Docs (docs/command-line-slangc-reference.md, docs/user-guide/a3-02-reference-capability-atoms.md)
- Regenerated to list
SPV_KHR_abortandspvAbortand to dropSPV_KHR_shader_abort.
Test directives (tests/spirv/abort*.slang, tests/diagnostics/abort-*.slang, tests/autodiff/abort-in-differentiable*.slang)
-skip-spirv-validationremoved from 12 abort tests;tests/spirv/abort.slangCHECK updated toOpExtension "SPV_KHR_abort"; explanatory notes about the validator-not-knowing-the-extension are dropped.
Findings (1 total)
| Severity | Location | Finding |
|---|---|---|
| 🔵 Question | source/slang/slang-capabilities.capdef:1049 (and 4 sibling sites) |
VK_KHR_shader_abort doc-comment references not renamed alongside the SPIR-V atom |
Motivation
The abort intrinsic emits the SPIR-V
OpAbortKHRinstruction. That instruction is provided by the Khronos SPIR-V extension namedSPV_KHR_abort, but this PR previously emitted and documented the non-registry spellingSPV_KHR_shader_abort. A concrete failure mode istests/spirv/abort.slang:24: the test expectsOpCapability AbortKHRand anOpExtensiondeclaration before theOpAbortKHRinstructions, and the validator only recognizes the canonicalSPV_KHR_abortextension name.The same spelling mismatch also existed in Slang's capability source of truth, so
source/slang/slang-capabilities.capdefexposed a SPIR-V extension atom namedSPV_KHR_shader_abortwhileemitAbortneeded to emitSPV_KHR_abort. That left two names for one SPIR-V extension.Proposed solution
Use the Khronos extension name everywhere Slang models the SPIR-V extension, and keep the SPIR-V capability as a separate capability atom:
source/slang/slang-emit-spirv.cpp:4796declaresOpExtension "SPV_KHR_abort"when emittingOpAbortKHR.source/slang/slang-capabilities.capdef:580defines the SPIR-V extension atom asSPV_KHR_abort.source/slang/slang-capabilities.capdef:701addsspvAbortfor the SPIR-VAbortKHRcapability, inheriting fromSPV_KHR_abort.source/slang/slang-capabilities.capdef:1059mapsGL_EXT_shader_abortto GLSL's_GL_EXT_shader_abortatom or SPIR-V'sspvAbortatom, preserving the existingabort = GL_EXT_shader_abortcompound capability atsource/slang/slang-capabilities.capdef:2423.With the extension name corrected, the abort-related SPIR-V tests no longer need to bypass SPIR-V validation.
Change summary
source/slang/slang-emit-spirv.cpp:emitAbortnow emitsSPV_KHR_abortwhile still requiringSpvCapabilityAbortKHRand emittingOpAbortKHRfrom the already-legalized abort message payload.source/slang/slang-capabilities.capdef: renames the SPIR-V extension atom toSPV_KHR_abort, addsspvAbortforAbortKHR, updatesGL_EXT_shader_abortto usespvAbortfor SPIR-V, and keeps the publicabortcompound capability unchanged.docs/user-guide/a3-02-reference-capability-atoms.mdanddocs/command-line-slangc-reference.md: regenerate the capability and command-line references so they listSPV_KHR_abortandspvAbort, and no longer documentSPV_KHR_shader_abort.tests/spirv/abort.slang: updates the FileCheck expectation toOpExtension "SPV_KHR_abort".Abort-related tests under
tests/spirv/,tests/diagnostics/, andtests/autodiff/: remove-skip-spirv-validationnow that the emitted extension string is accepted by the bundled SPIR-V validator.tests/diagnostics/abort-unsupported-target.slang:2also updates its explanatory capability comment.Concepts and vocabulary
SPIR-V extension atom: a
SPV_*capability definition inslang-capabilities.capdefthat names a SPIR-V extension such asSPV_KHR_abort.SPIR-V capability atom: a lower-camel
spv*capability definition that models a SPIR-VOpCapability, such asspvAbortforAbortKHR.abortcompound capability: the user-facing capability used by the abort intrinsic. It remains an alias forGL_EXT_shader_abort, which now expands to the GLSL extension atom or the SPIR-VspvAbortatom depending on target.Process report
The first issue was the emitted SPIR-V module shape.
source/slang/slang-emit-spirv.cpp:4794handles anAbortIR instruction after SPIR-V legalization has already packed the abort message into a payload struct. The input shape is correct:emitAbortreceives a validAbortinstruction and still requiresSpvCapabilityAbortKHRatsource/slang/slang-emit-spirv.cpp:4797. The bug was not the IR producer or payload representation; it was the extension declaration string emitted beside a validOpAbortKHR. UpdatingensureExtensionDeclarationtoSPV_KHR_abortfixes the backend mapping at the point where SPIR-V syntax is emitted.That fix exposed a second source-of-truth issue in the capability model. Leaving
SPV_KHR_shader_abortinslang-capabilities.capdefwould make Slang document and accept a SPIR-V extension atom whose name does not match the emitted extension. The principled split is to useSPV_KHR_abortfor the extension atom andspvAbortfor the SPIR-VAbortKHRcapability.GL_EXT_shader_abortremains the cross-target capability bridge, now mapping to_GL_EXT_shader_abortfor GLSL andspvAbortfor SPIR-V.The test changes follow from the corrected output.
tests/spirv/abort.slang:25now checks the canonical extension string, and the abort tests that previously passed-skip-spirv-validationcan use normal SPIR-V validation because the bundled validator recognizesSPV_KHR_abort. These are not guards for malformed input; they remove a validation bypass that existed only because Slang was emitting a non-registry extension spelling.