Capabilities: error on capabilities incompatible with compilation target (fixes #4422) - #11225
Capabilities: error on capabilities incompatible with compilation target (fixes #4422)#11225zangold-nv wants to merge 17 commits into
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:
📝 WalkthroughWalkthroughAdds target capability checks before entry-point validation. The change defines E36121, promotes E36112 to an error, adds GLSL-based target classification, integrates per-target validation, and expands capability regression tests. ChangesCapability compatibility validation against compilation targets
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 |
![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: 23be7610-ffd4-4976-835d-148a299e21e7
📒 Files selected for processing (7)
source/slang/slang-check-shader.cppsource/slang/slang-diagnostics.luasource/slang/slang-target.cppsource/slang/slang-target.htests/language-feature/capability/conflicting-profile-stage-for-entry-point.slangtests/language-feature/capability/incompatible-capability-for-target.slangtests/language-feature/capability/specializeTargetSwitch.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.
…shader-slang#4422) Two validation gaps are addressed: 1. Requesting a capability whose target family conflicts with the code-gen target (e.g. `-capability spirv_1_5 -target hlsl`) now emits error E36121 instead of silently ignoring the capability. The new `TargetRequest::checkCapabilities(DiagnosticSink*)` method is called once per target from `FrontEndCompileRequest::checkEntryPoints`. GLSL targets are exempt for SPIRV-family capabilities since those are intentionally auto-converted to glsl_spirv equivalents. 2. Promoting the stage-vs-profile mismatch diagnostic (entry-point-and-profile-are-incompatible, E36112) from a warning to an error, so that e.g. compiling with profile `cs_5_0` while the entry point is a fragment shader is now a hard error. Tests: - New test: tests/language-feature/capability/incompatible-capability-for-target.slang - Updated: conflicting-profile-stage-for-entry-point.slang (warning → error) - Updated: specializeTargetSwitch.slang (remove now-invalid mismatched capability)
- Use containsKey(CapabilityAtom::spirv) instead of getCompileTarget()==spirv
in the GLSL exemption check, to avoid relying on unordered dictionary
iteration order for capabilities spanning multiple target families.
- Add sync comment noting that the isGLSLTarget logic mirrors getTargetCaps().
- Expand incompatible-capability-for-target.slang to cover:
- -ignore-capabilities suppressing E36121
- GLSL target + spirv_1_5 exemption (should NOT error)
- CPP target + image_loadstore (should error)
- Document that cross-target aliases like sm_6_0 are designed to include
SPIRV paths and intentionally do not trigger E36121.
…tic message - Pass getLinkage()->m_optionSet instead of per-target optionSet to maybeDiagnose, consistent with all other capability diagnostics in slang-check-shader.cpp. This ensures global -ignore-capabilities suppresses E36121 as expected. - Move requestedCap and target names into the primary diagnostic message rather than a location-less span, so the capability name always appears in the error output. - Add capability name anchors to test CHECK_ERR/CHECK_ERR_CPP cases, and positive anchors (result code / OpEntryPoint) to the success -NOT cases. - Update slang-target.h doc comment to reflect that maybeDiagnose may suppress the diagnostic when capability checking is disabled. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Extract shared isGLSLBasedTarget() private helper on TargetRequest, eliminating the duplicate isGLSLTarget switch that existed in both getTargetCaps() and checkCapabilities(). The comment flagging the sync risk is removed together with the duplicate code. - Add test case: HLSL-specific capability (_sm_6_6) on a GLSL target triggers E36121, covering the missing equivalence-class direction. - Clarify comment on SPIRV success test case: -target spirv without -emit-spirv-directly takes the GLSL-SPIRV path internally, which is why SPIRV capability atoms are auto-converted rather than rejected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Root cause is downstream in SlangPy — this PR is working as designed.
🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify. |
…nked SlangSession::create_session requested the hlsl_nvapi Slang capability unconditionally. The sgl NVAPI Slang module that provides it is created and linked into programs only under SGL_HAS_NVAPI && DeviceType::d3d12, so the capability request was strictly broader than the module backing it. (The separate SGL_ENABLE_NVAPI compiler define, which shader code branches on, is already gated on that same predicate.) Slang 2026.12, the currently pinned version, tolerates the mismatch silently. shader-slang/slang#11225 rejects it: error[E36121]: requested capability 'hlsl_nvapi' is incompatible with compilation target 'spirv' Gate the request on the predicate already used at the module create and link sites so they cannot drift apart. The non-d3d12 device types this function maps to a target -- spirv, metal, wgsl, ptx and host-callable -- have no NVAPI module linked; of those, only spirv is covered by a job in the dispatch matrix that surfaced this, so that is the only target observed failing. Both halves of the predicate are load-bearing. SGL_HAS_NVAPI is OFF wherever slang-rhi does not build its NVAPI target -- which requires Windows, x86_64 and a D3D backend -- and there the module is never created or linked. Where it is ON, the capability is genuinely backed for d3d12 but not for other device types, so the device check is still required. Fixes #1087
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 Plus
Run ID: d89ac489-0eba-4d56-9615-6de6d29472e8
📒 Files selected for processing (7)
source/slang/slang-check-shader.cppsource/slang/slang-diagnostics.luasource/slang/slang-target.cppsource/slang/slang-target.htests/language-feature/capability/conflicting-profile-stage-for-entry-point.slangtests/language-feature/capability/incompatible-capability-for-target.slangtests/language-feature/capability/specializeTargetSwitch.slang
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Status unchanged:
One note on the 🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify. |
Session-level capabilities (set via SessionDesc.compilerOptionEntries) are inherited by every target through CompilerOptionSet::inheritFrom(). Multi-backend frameworks like SGL/SlangPy legitimately add hlsl_nvapi at the session level to enable it for D3D12 targets while also compiling the same session for Vulkan/SPIRV, where hlsl_nvapi is not valid. Previously checkCapabilities() read from targetOptionSet.getArray() which includes inherited capabilities, and so E36121 fired incorrectly for every SPIRV compile in an SGL session, breaking the SlangPy CI tests. Fix: skip any capability that also appears in the linkage-level option set (getLinkage()->m_optionSet), which holds exactly what was set at the session level. Capabilities set explicitly for a specific target via TargetDesc.compilerOptionEntries or addTargetCapability() go directly into the target's option set and are NOT in the linkage option set, so they continue to be checked. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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-target.cpp (1)
67-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
TargetRequest::isGLSLBasedTarget().Add a complete-sentence function comment. State that it classifies targets that use GLSL capability semantics. State why indirect SPIR-V uses that classification while direct SPIR-V does not.
As per coding guidelines, “Comment C++ functions as complete sentences: what, then why.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf65b84c-c73a-44aa-82d7-e33b4b69065f
📒 Files selected for processing (3)
source/slang/slang-target.cpptests/language-feature/capability/session-level-capability-not-flagged.slangtools/slang-unit-test/unit-test-session-capability-not-flagged.cpp
|
Re-verified at the new head
I have not pushed anything to your branch and won't — 🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 Plus
Run ID: ca4ebb60-1a1f-40c4-8d77-f1a5b2d12b25
📒 Files selected for processing (9)
source/slang/slang-check-shader.cppsource/slang/slang-diagnostics.luasource/slang/slang-target.cppsource/slang/slang-target.htests/language-feature/capability/conflicting-profile-stage-for-entry-point.slangtests/language-feature/capability/incompatible-capability-for-target.slangtests/language-feature/capability/session-level-capability-not-flagged.slangtests/language-feature/capability/specializeTargetSwitch.slangtools/slang-unit-test/unit-test-session-capability-not-flagged.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…name in diagnostic, expand test coverage - Extract shared `decodeCapabilityOption` static method used by both `getTargetCaps()` and `checkCapabilities()`, eliminating duplicated Int/String decode logic. - Fix E36121 diagnostic to report the user-specified target name (e.g. 'spirv') rather than the capability-set's target atom (which returns 'glsl' on the GLSL-SPIRV pipeline), by using TypeTextUtil::getCompileTargetName(asExternal(getTarget())). - Add test case for -emit-spirv-via-glsl: SPIRV cap on GLSL-SPIRV pipeline is auto-converted, no E36121. - Add Metal/_sm_6_6, WGSL/spirv_1_5, CUDA/_GLSL_460 error test cases verifying the target name in the diagnostic. - Fix wrong comment about -target spirv default path (it is direct emit, not via-GLSL). - Add -emit-spirv-directly test confirming no false positive.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
tangent-vector
left a comment
tangent-vector
left a comment
There was a problem hiding this comment.
I have several fine-grained feedback points / change requests, but also one high-level concern about this entire PR:
I presume that we already have logic in the Slang compiler that translates the -capability options that a user specified on the command line into capability atoms that get turned into a capability set, for use downstream... but then this PR seems to introduce an entirely new code path that iterates over command-line options and handles translation of those options over to capabilities (including the string-to-integer mapping which, again, other logic would presumably already have done). Ultimately it does all of this just so that it can do an atom-by-atom iteration and check individual atoms against another capability set, but this seems like exactly the kind of operation that should be available and implemented more fundamentally for the sets themselves.
What I'd expect to see here is a stronger architectural direction, where we recognize capabilities coming from different sources, and store those capability sets with their provenance so that they can feed into downstream operations like validation. We know that capabilities can come from session-level options, target-level options, entry-point attributes, inferred requirements of an entry-point function and its call graph, etc. What we need is a coherent description for how those capability sets should be combined, and what validation should be applied in cases where something inconsistent/incoherent arises between all those different sources.
This PR seems like a clear example of an agent creating a band-aid fix, rather than addressing an actual issue in SW architecture.
|
|
||
| void TargetRequest::checkCapabilities(DiagnosticSink* sink) | ||
| { | ||
| if (!sink) |
There was a problem hiding this comment.
This early-out should either:
- Be an assertion, to make explicit that this is a precondition of the function and detect precondition violations
- Include a comment with a rock-solid explanation for why (1) this situation is allowed/expected to arise in ordinary execution, and (2) this particular handling of the situation is 100% correct and the only logical thing to do
The above rules should apply to every single early-out from a function/loop/etc. that appears in any PR for the Slang codebase. This is not a one-off problem; agents love to write garbage like this.
| // For GLSL-based targets, SPIRV version and extension atoms are intentionally | ||
| // converted to their GLSL-SPIRV equivalents by getTargetCaps(), so they are | ||
| // not an error here. |
There was a problem hiding this comment.
This seems like a bug, unrelated to the current PR.
If there is a rationale for why the SPIRV version atoms and the GLSL-SPIRV version atoms are distinct, is that documented somewhere? And if it is an intentional choice with a documented rationale... is it actually okay to then treat both cases as equivalent here?
@nv-slang-bot If the answer is "there is no clear rationale" then open an issue to note that the duplication there is probably a mistake, and we should consolidate on a single set of capability atoms for SPIRV versions, with the GLSL ones just being turned into aliases over the SPIRV ones.
There was a problem hiding this comment.
There is a documented, intentional rationale, so per your conditional this doesn't warrant a "the duplication is a mistake" issue — but the maintainability smell you're sensing is real, and I'll spell out where the line is.
Why the two families are distinct (documented): they belong to different code-gen targets, not two spellings of the same thing.
def _spirv_1_0 : spirv;…_spirv_1_6— rooted in thespirvtarget = direct SPIR-V codegen (slang-capabilities.capdef:162-168).def glsl_spirv_1_0 : glsl;…glsl_spirv_1_6— rooted in theglsltarget = SPIR-V produced via GLSL → glslang (slang-capabilities.capdef:131-155).
The rationale is stated in the capdef itself at slang-capabilities.capdef:126-127:
// Capabilities that stand for target SPIR-V versions for the GLSL backend.
// These are not compilation targets. We will convertspirvtoglsl_spirv_during compilation.
and per-atom ("Represents SPIR-V 1.5 through glslang" vs "Represents SPIR-V 1.5 version"), which surfaces in the generated reference docs/user-guide/a4-02-reference-capability-atoms.md. The distinction was introduced deliberately in #4241.
A CapabilitySet partitions its atoms by target family (m_targetSets keyed on CapabilityAtom::spirv / CapabilityAtom::glsl, slang-capability.cpp:1577-1579), so a given version atom lives in exactly one family. spirv_1_5 means "SPIR-V 1.5 on the direct path"; glsl_spirv_1_5 means "SPIR-V 1.5 on the glslang path" — genuinely different pipelines with different feature availability. Concretely, _GL_EXT_ray_query : _GLSL_460 + glsl_spirv_1_4 (slang-capabilities.capdef:1007) requires the GLSL-path 1.4 atom specifically, with the inline note "spirv_1_4 is required due to glslang bug which enables SPV_KHR_ray_tracing regardless of context" — that requirement is only meaningful on the glslang path. And the sm_6_x version aliases carry both families side-by-side (sm_6_0_version = _sm_6_0 | _GLSL_450 | spirv_1_3 | …, slang-capabilities.capdef:1792-1799), i.e. they coexist rather than substitute for each other.
Is it OK to treat them as equivalent here? Yes — and note checkCapabilities isn't asserting they're equal, it's exempting SPIRV version/extension atoms from the incompatibility error on GLSL-based targets. That exemption is correct because getTargetCaps() already converts them: addSpirvVersionFromOtherAsGlslSpirvVersion (slang-capability.cpp:1575-1611, called at slang-target.cpp:149 and :229) moves a requested spirv_1_x into the glsl family as glsl_spirv_1_x. So -capability spirv_1_5 -target glsl is genuinely not incompatible — the conversion happens — and flagging it would be a false positive. The exemption is the right call.
On the consolidation proposal specifically: turning the glsl_spirv_* atoms into aliases over the spirv_* ones as literally described would re-root them into the spirv target family, which (a) breaks GLSL-path gates like the _GL_EXT_ray_query one above, and (b) leaves addSpirvVersionFromOtherAsGlslSpirvVersion with no distinct target to convert into. So that exact remedy isn't viable.
Where I agree there's a smell: two parallel version ladders kept in manual lockstep, plus the conversion done by raw enum-offset arithmetic (maybeConvertSpirvVersionToGlslSpirvVersion, slang-capability.cpp:1565-1573) that silently assumes _spirv_1_x and glsl_spirv_1_x stay contiguous and aligned in the generated enum. That's fragile, but it's a maintainability/robustness concern, not evidence of accidental duplication. If you'd like, I can file a narrower tracking issue for making that mapping declarative (or asserting the ordering invariant) rather than positional — just say the word and I'll open it and link it here.
🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify.
| // Capabilities inherited from the session-level option set (via inheritFrom) are | ||
| // intentionally broad — a multi-backend application may set e.g. hlsl_nvapi at | ||
| // the session level to enable it for D3D12 targets while also compiling for | ||
| // Vulkan/SPIRV in the same session. Only flag capabilities that were explicitly | ||
| // requested for this specific target, not ones that arrived via session inheritance. |
There was a problem hiding this comment.
This seems like a dangerous policy decision to be introducing by fiat here. Is the stated policy documented somewhere, or is it just a "do what I mean" policy choice being made locally to try and not break some existing use cases?
AFAICT the whole point of this PR is to be more strict about how we diagnose situations where capabilities are being requested that are logically inconsistent with one another or the target, so it seems to actively go against that goal to sweep some cases of inconsistency under the rug and not others.
| // Vulkan/SPIRV in the same session. Only flag capabilities that were explicitly | ||
| // requested for this specific target, not ones that arrived via session inheritance. | ||
| auto sessionCapArray = getLinkage()->m_optionSet.getArray(CompilerOptionName::Capability); | ||
| auto isSessionLevelCap = [&](const CompilerOptionValue& atomVal) -> bool |
There was a problem hiding this comment.
Actual question: is the codebase-wide policy in favor of using locally-defined lambdas for helper routines rather than defining them as file-level static functions? I have a personal preference for file-level statics because they make the contract of an operation explicit, force careful thought about naming, and encourage the author to write a thoughtful documentation comment explaining what they are doing.
If the codebase policy is in favor of this lambda-based idiom, I'll take it up with the policy separately. If there is no such policy, then I'd push for an actual subroutine instead of a lambda, where possible.
| if (isGLSLTarget && toAdd.getCapabilityTargetSets().containsKey(CapabilityAtom::spirv)) | ||
| continue; | ||
|
|
||
| if (!cookedCaps.isIncompatibleWith(toAdd)) |
There was a problem hiding this comment.
Suggest adding an isCompatibleWith helper to the capability-set type used here, rather than use a double-negative in a condition like this.
…ce model Address tangent-vector's architectural review of the E36121 check: rather than TargetRequest::checkCapabilities() re-decoding raw -capability option values and reconstructing "did this come from the session or the target" by comparing option sets after the fact, capability requirements are now tagged with where they came from at the point they are produced, and validated through one shared primitive. - slang-capability.h/.cpp: add CapabilitySource (SessionOption / TargetOption / EntryPointRequirement) and SourcedCapabilityRequirement, plus findIncompatibleCapabilityRequirements(), the single place that checks a list of sourced capability requirements against a target's capabilities. - slang-compiler-options.h/.cpp: CompilerOptionValue carries a capabilitySource field for Capability-kind entries, set once at the producer (CompilerOptionSet::load()/addCapabilityAtom()) and preserved automatically through inheritFrom/overrideWith merges instead of being reconstructed later. - slang-target.cpp: checkCapabilities() now reads each entry's capabilitySource directly and calls the shared helper; the hand-rolled isSessionLevelCap lambda is gone. Replaced the null-sink early return with a release assert (every call site has a live sink) and gave isGLSLBasedTarget() a what-then-why doc comment. - slang-check-shader.cpp: validateEntryPoint()'s target-vs-entry-point compatibility test now goes through the same findIncompatibleCapabilityRequirements() helper checkCapabilities() uses, so a target's own capabilities and an entry point's required capabilities are checked by one shared rule. The GLSL-target SPIRV exemption in checkCapabilities() is left as-is behaviorally (still broader than what getTargetCaps() actually converts) with a TODO pointing at a follow-up issue, per review discussion: it's a pre-existing, documented atom-family distinction rather than something to fix as a side effect here. Fixes shader-slang#4422
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…fix stale comment - checkCapabilities() now latches on a per-target bool guarded by m_mutex, so a persistent session that loads many modules against the same target doesn't re-derive and re-diagnose the same target-capability incompatibility on every module load (checkEntryPoints() runs once per FrontEndCompileRequest, i.e. once per module load, not once per target's lifetime). - decodeCapabilityOption() now optionally writes the capability's display name through an out-parameter, so checkCapabilities() no longer re-opens a second switch (atomVal.kind) that duplicated (with divergent guards) the one already inside the shared decode helper. The redundant SLANG_CAPABILITY_UNKNOWN / CapabilityName::Invalid pre-checks are gone too: decodeCapabilityOption() already produces an empty CapabilitySet for those cases, which the existing toAdd.isEmpty() check already skips. - isGLSLBasedTarget()'s doc comment no longer says "keep in sync with getTargetCaps()" -- after this PR's earlier commit, getTargetCaps() reads its own isGLSLTarget local from isGLSLBasedTarget() rather than duplicating the classification, so isGLSLBasedTarget() is the single source of truth, not one of two things to keep in sync. - Filled in the TODO's placeholder issue link (issues/NNNN) with the real tracking issue (shader-slang#12703) for the over-broad GLSL/SPIRV-extension exemption.
The test row it labels was changed to target cpp (dropping the stale -capability image_loadstore that now correctly triggers E36121), but the filecheck label was left as CHECK_METAL, which is misleading -- there is no Metal target in this file. Renamed to CHECK_CPP to match what the row actually tests.
…y decode path Every existing test drives -capability from the CLI, which always produces an Int-kind CompilerOptionValue (slang-options.cpp's addCapabilityAtom -> CompilerOptionValue::fromEnum), and the existing C++ unit tests both use the Int-kind form too. decodeCapabilityOption()'s String-kind branch -- reachable only via the C++ API (CompilerOptionValueKind::String with a capability name string) -- had no regression coverage. Added a unit test that requests an incompatible capability by name as a String-kind TargetDesc.compilerOptionEntries entry and confirms E36121 still fires.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…plication Extract shared resolveHumaneLoc/makeDiagnosticSpan helpers in richDiagnosticThunk to eliminate the duplicated primary/secondary span field-mapping logic, and add rock-solid rationale comments to the early-out guards in richDiagnosticThunk, installDiagnosticCallback, and DiagnosticSink::diagnoseRichImpl, per tangent-vector's recurring review feedback on PRs shader-slang#11225 and shader-slang#12421 about unexplained early-outs and duplicated logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up fix for the
|
… is requested at both session and target scope CompilerOptionSet::add()'s duplicate-merge branch (replaceDuplicate=true, used by overrideWith()) only copied intValue2/stringValue2 into the existing entry, not capabilitySource. Linkage::addTarget() builds a target's option set as inheritFrom(m_optionSet) (copies session-level entries, tagged SessionOption) followed by overrideWith(targetOptions) (merges in target-level entries, tagged TargetOption). When the same capability atom is requested at both scopes, the second (target-level) entry is treated as a duplicate of the first (session-level) one rather than a new entry, so the existing SessionOption tag silently won -- a genuinely binding target-level request was tolerated by checkCapabilities() as if it were a best-effort session broadcast, with no E36121. Copy capabilitySource in the same branch so a later overrideWith() entry's source correctly replaces the inherited one, matching overrideWith()'s documented "copy settings from other, and replace the current setting" semantics. Added a unit test (sameCapabilityAtBothScopesStillFlagged) that requests the same capability at both scopes and asserts E36121 still fires.
… E36121 on later module loads The latch added in 9ffd7fd ("cache checkCapabilities()... so a persistent session doesn't re-diagnose the same target-capability incompatibility on every module load") was wrong: m_capabilitiesChecked lives on the TargetRequest, which is shared across the Linkage's whole session lifetime, not per-compile. So only the *first* loadModule() call against a misconfigured target got E36121; every later load against that same target silently returned no diagnostic in its own (independently inspected) diagnostic blob, even though that load's compile is just as invalid. The "duplicate diagnostics across loads" observation this was fixing was never actually a bug: every other target-config diagnostic in this file (e.g. the profile/stage-mismatch check in validateEntryPoint()) runs uncached on every FrontEndCompileRequest, and that is correct -- a fresh DiagnosticSink is created per module load, and each one must independently reflect whether that load's compile is valid. Restoring that behavior here.
…not being flagged sm_6_0 (and aliases like it) intentionally spans HLSL, SPIRV, CUDA, Metal, CPP and LLVM -- it must not be flagged as incompatible on any target it spans. This was previously documented only as a prose comment; a regression here would have gone undetected. Added SIMPLE tests requesting sm_6_0 on both an HLSL and a SPIRV target and asserting no E36121 on either.
…int's use of findIncompatibleCapabilityRequirements Note that the single-element SourcedCapabilityRequirement wrapping at this call site only exists to route through the same shared compatibility primitive checkCapabilities() uses; source and label aren't read here, only the returned count is.
There was a problem hiding this comment.
Verdict: 🟡 Minor issues — 0 bugs, 3 gaps (1 test, 2 clarity/maintainability)
This PR adds error E36121 for -capability requests incompatible with the code-gen target, promotes E36112 (profile/stage mismatch) from warning to error, and threads a CapabilitySource provenance (SessionOption/TargetOption/EntryPointRequirement) through CompilerOptionValue so session-scope requests are tolerated while target-scope ones are diagnosed. Memory-safety and IR/check-correctness passes found no defects; the session-vs-target merge logic is correct and unit-tested. Findings are limited to one vacuous test case and two clarity/maintainability items.
Changes Overview
Capability provenance model (slang-capability.h/.cpp, slang-compiler-options.h/.cpp)
- Adds
CapabilitySourceenum +SourcedCapabilityRequirement, tagsCompilerOptionName::Capabilityentries with their origin atload()/addCapabilityAtom(), and carries the tag throughinheritFrom/overrideWithmerges. New shared helperfindIncompatibleCapabilityRequirements.
Target-capability validation (slang-target.cpp/.h, slang-check-shader.cpp, slang-diagnostics.lua)
- New
TargetRequest::checkCapabilities(sink)(called once per target incheckEntryPoints()) diagnoses target-scope incompatible caps as E36121, exempting GLSL-based targets for SPIRV caps (auto-converted). ExtractsisGLSLBasedTarget()/decodeCapabilityOption()as single-source-of-truth helpers.validateEntryPointreroutes its incompatibility check through the shared helper. E36112 promoted warning→error.
Provenance plumbing (slang-global-session.cpp, slang-session.cpp, slang-linkable.cpp)
- Session-desc load →
SessionOption; target-desc load andlinkWithOptions→TargetOption.
Tests (incompatible-capability-for-target.slang new, unit-test-session-capability-not-flagged.cpp new, conflicting-profile-stage-for-entry-point.slang, specializeTargetSwitch.slang)
- Cross-target E36121 matrix,
-ignore-capabilitiessuppression, GLSL auto-conversion, cross-target aliases; 4 unit tests for session-vs-target scope handling; E36112 expectation updated warning→error.
Findings (3 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | tests/language-feature/capability/incompatible-capability-for-target.slang:16 |
CHECK_VIA_GLSL_OK has only a -NOT directive — the sole test of the SPIRV-via-GLSL branch passes vacuously |
| 🟡 Gap | source/slang/slang-compiler-options.h:181 |
capabilitySource merged unconditionally for all option kinds via a hand-maintained field list |
| 🟡 Gap | source/slang/slang-capability.h:539 |
CapabilitySource enum mixes *Option/*Requirement naming conventions |
reviewed: aa4fa4d · diff sha256 617820238a6c
| // cap is auto-converted to a glsl_spirv_* equivalent by getTargetCaps(), so no error. | ||
| // This exercises the isGLSLBasedTarget() + SPIRV-cap exemption branch. | ||
| //TEST:SIMPLE(filecheck=CHECK_VIA_GLSL_OK): -target spirv -emit-spirv-via-glsl -entry main -stage compute -capability spirv_1_5 | ||
| // CHECK_VIA_GLSL_OK-NOT: E36121 |
There was a problem hiding this comment.
🟡 Gap: CHECK_VIA_GLSL_OK is a vacuous (negative-only) check
This case is the only test that exercises the new isGLSLBasedTarget() SPIRV-via-GLSL branch (case SPIRV/SPIRVAssembly: return !optionSet.shouldEmitSPIRVDirectly();) — the comment above it even says "This exercises the isGLSLBasedTarget() + SPIRV-cap exemption branch." But its only directive is -NOT: E36121 with no positive assertion. If the -emit-spirv-via-glsl pipeline fails or emits nothing for any unrelated reason, the test still passes because E36121 is merely absent, so the branch it is meant to guard would not actually be verified.
Every other OK case has a positive check (CHECK_OK/CHECK_SPIRV_DIRECT_OK assert OpEntryPoint; CHECK_IGNORE/CHECK_GLSL_OK assert result code = 0); only this one does not.
Suggestion: Add a positive assertion alongside the -NOT, e.g.
// CHECK_VIA_GLSL_OK: result code = 0
// CHECK_VIA_GLSL_OK-NOT: E36121
| { | ||
| (*v)[index].intValue2 = element.intValue2; | ||
| (*v)[index].stringValue2 = element.stringValue2; | ||
| (*v)[index].capabilitySource = element.capabilitySource; |
There was a problem hiding this comment.
🟡 Gap: capabilitySource is merged unconditionally for every option kind, via a hand-maintained field list
This replaceDuplicate branch merges an enumerated subset of CompilerOptionValue's fields into an existing duplicate (intValue2, stringValue2, and now capabilitySource), rather than the whole value — the dedup key (intValue/stringValue) is deliberately excluded. Two concerns:
- Provenance is copied for all option names, not just
Capability.capabilitySourceis documented as "Meaningless for every other option name," so copying it for e.g. a duplicateProfileentry is harmless today, but it reads as unintended since the field's meaning is capability-specific. - Standing fragility. Any future semantically-significant field added to
CompilerOptionValuemust be remembered here or it is silently dropped on dedup. AddingcapabilitySourceto this list is the correct fix (verified bysameCapabilityAtBothScopesStillFlagged), but the omission of other fields is not obviously deliberate to a future maintainer.
Suggestion: Either guard the assignment so intent is explicit —
if (name == CompilerOptionName::Capability)
(*v)[index].capabilitySource = element.capabilitySource;— or add a one-line comment stating that on a duplicate only these payload fields are merged (the match key intentionally is not), so the field-by-field copy reads as intentional.
|
|
||
| /// Identifies which part of a compile produced a capability requirement that is being | ||
| /// checked against a target's capabilities. | ||
| enum class CapabilitySource |
There was a problem hiding this comment.
🟡 Gap: CapabilitySource enum mixes two naming conventions
SessionOption/TargetOption name the mechanism (a compiler option), while EntryPointRequirement names the concept (a requirement). Since the enum represents "where a capability requirement came from," a reader has to reconcile whether these are three kinds of option-source or three kinds of requirement-source. Aligning the case names on one axis (all as request scopes, or all as requirement sources) would let the exhaustive set read as one decomposition. Cosmetic — the per-case doc comments are otherwise thorough.
3 participants
Summary
Previously, passing a capability flag that is incompatible with the
compilation target (e.g.
-capability spirv_1_5 -target hlsl) wassilently ignored. This PR detects that mismatch and emits an error.
requested capability 'X' is incompatible with compilation target 'Y'— fires when an explicitly requested capability (via-capability) is incompatible with the code-gen target family (e.g. SPIRV cap on HLSL target, GLSL cap on CPP target)-entry vsmain -profile ps_6_0) now produces an error instead of a warning-ignore-capabilitiessuppresses E36121, consistent with all other capability diagnosticsglsl_spirv_*equivalents rather than rejectedsm_6_0, which spans both HLSL and SPIRV) are not flagged — they are valid on any of their target familiesImplementation notes
TargetRequest::checkCapabilities(DiagnosticSink*)method, called once per target inFrontEndCompileRequest::checkEntryPoints()TargetRequest::isGLSLBasedTarget()helper shared bygetTargetCaps()andcheckCapabilities()to avoid duplicating the GLSL-target detection logicCapabilityTargetSets::containsKey(CapabilityAtom::spirv)used for the GLSL exemption check (robust against unordered-map iteration order)Test plan
tests/language-feature/capability/incompatible-capability-for-target.slang(new)-ignore-capabilities→ suppressedconflicting-profile-stage-for-entry-point.slang: updated for E36112 promotionspecializeTargetSwitch.slang: removed stale-capability image_loadstorefrom the CPP test case that now correctly triggers E36121Fixes #4422