render-test: resolve cooperative-matrix-2 sub-feature names to a real RHI feature - #12735
Conversation
… RHI feature -render-feature names were resolved in two places that disagreed. Option parsing accepted any name starting with "cooperative-matrix-" via a work-around, on the stated grounds that such a test would be "gracefully skipped at runtime if hardware doesn't support it". The runtime requirement check used a separate mapping that only knew the names generated from SLANG_RHI_FEATURES, so those names resolved to Feature::_Count, and Device::hasFeature() returns false for _Count on every device. The tests were therefore skipped unconditionally -- including on hardware that does implement VK_NV_cooperative_matrix2 -- rather than only where support was missing. Five such names are used by 23 test invocations across 6 files, none of which have ever executed. Introduce getRenderFeatureFromName() as the single mapping from a feature name to an rhi::Feature, and use it for both option validation and the runtime requirement check so the two can no longer disagree. slang-rhi exposes VK_NV_cooperative_matrix2 only as the single cooperative-matrix-2 feature, so resolve each sub-feature name onto it. Names that cannot be resolved are once again rejected at option-parse time, which the removed work-around had suppressed for anything cooperative-matrix-prefixed.
📝 WalkthroughWalkthroughThe render-test tool now centralizes feature-name resolution. It supports cooperative-matrix-2 aliases, validates names through the shared resolver, and uses the resolver when building required render features. ChangesRender feature resolution
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR centralizes cooperative-matrix feature resolution and restores correct test gating; only a minor documentation example remains advisable, with no actionable merge-blocking risk. 🚥 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.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fd18a65-7549-4a93-ab65-04db6159a825
📒 Files selected for processing (3)
tools/render-test/options.cpptools/render-test/options.htools/render-test/render-test-main.cpp
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| /// Return the `rhi::Feature` a `-render-feature` name refers to, or `rhi::Feature::_Count` if the | ||
| /// name is not recognized. | ||
| /// | ||
| /// This is the single place that maps a test's feature name onto an RHI feature, so that option | ||
| /// parsing (which rejects unknown names) and the runtime requirement check (which decides whether | ||
| /// to skip a test) can never disagree. Besides the names generated from `SLANG_RHI_FEATURES`, it | ||
| /// resolves the individual `VK_NV_cooperative_matrix2` sub-feature names used by tests -- slang-rhi | ||
| /// exposes that extension only as the single `cooperative-matrix-2` feature, so each sub-feature | ||
| /// name maps onto it. | ||
| rhi::Feature getRenderFeatureFromName(const Slang::UnownedStringSlice& featureName); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add a concrete mapping example.
Document one alias mapping, such as "cooperative-matrix-reductions" mapping to rhi::Feature::CooperativeMatrix2. This makes the non-trivial alias behavior explicit.
As per coding guidelines, “Include a concrete example for non-trivial behavior.”
Source: Coding guidelines
There was a problem hiding this comment.
Verdict: ✅ Clean — no significant issues found (one optional clarity note inline)
This PR unifies feature-name→rhi::Feature resolution in the render-test tool into a single function getRenderFeatureFromName(), used by both option-parse validation and the runtime requirement check, and maps the five VK_NV_cooperative_matrix2 sub-feature names onto rhi::Feature::CooperativeMatrix2. This restores execution for 23 previously-dead cooperative-matrix-* test invocations and re-enables loud parse-time rejection of unknown feature names.
Changes Overview
Feature-name resolution unification (tools/render-test/options.cpp, tools/render-test/options.h, tools/render-test/render-test-main.cpp)
- Before: two independent mappings — option parsing (
isValidFeatureName, which accepted anycooperative-matrix-*viastartsWithwith a warning) and the runtime check (_getFeatureFromName, built only fromSLANG_RHI_FEATURES). They disagreed: sub-feature names passed validation but resolved to the_Countsentinel at runtime, whichhasFeature()treats as false unconditionally, so the tests were always skipped. - After: one resolver in
options.cpp(exported viaoptions.h) maps the five sub-feature names toCooperativeMatrix2and everything else through the generated table, returning_Countonly for genuinely unknown names.isValidFeatureNamereduces togetRenderFeatureFromName(name) != _Count; the duplicate_getFeatureFromNameinrender-test-main.cppis deleted and its caller uses the shared resolver.
Verification performed by the review agents:
- Regression safety: all 23
cooperative-matrix-*invocations undertests/cooperative-matrix/use exactly one of the five mapped names, so the stricter parse-time rejection breaks no existing test. - Sentinel/memory safety:
_Countcan no longer reachrequiredFeatureList— names are validated at parse time through the same resolver before being stored, and the runtime check re-resolves the same validated names. The PR in fact closes a pre-existing path where_CountreachedhasFeature(_Count). - Removed warning: the old "not yet fully supported" diagnostic was tied to the broken workaround and is now obsolete; its removal is intended.
The primary sub-feature-resolution behavior is only exercisable on NVIDIA hardware implementing VK_NV_cooperative_matrix2 and cannot be validated in CI without such hardware — consistent with the author's stated caveat that 23 never-run invocations will begin running and should be watched on NVIDIA CI.
reviewed: 9271f8c · diff sha256 dc2df840f4eb
| UnownedStringSlice::fromLiteral("cooperative-matrix-reductions"), | ||
| UnownedStringSlice::fromLiteral("cooperative-matrix-tensor-addressing"), | ||
| }; | ||
| for (const auto& subFeature : kCooperativeMatrix2SubFeatures) |
There was a problem hiding this comment.
🔵 Clarity: alias-table precedence over the generated table hides a removal condition
The sub-feature alias loop runs before the SLANG_RHI_FEATURES-generated table below it. If slang-rhi ever adds any of these five names to the X-macro (i.e. exposes the granular sub-features), the alias here would shadow the generated entry and keep resolving the name to CooperativeMatrix2 — the granular feature would be unreachable through this resolver.
The PR description already states the intended remediation ("if slang-rhi later exposes the granular features, the alias entries are deleted and the generated table picks the names up"), but that dependency lives only in the PR body. A future maintainer reading the code sees why the aliases exist but not when they must be removed.
Suggestion: add a one-line comment on this block noting the aliases should be deleted once slang-rhi exposes these names directly, so the resolver returns the granular feature rather than being silently shadowed. Optional/non-blocking.
1. Motivation
Six test files gate themselves on cooperative-matrix-2 sub-features:
None of these 23 test invocations has ever executed, on any device. Not on lavapipe, not on
NVIDIA hardware that implements
VK_NV_cooperative_matrix2. They are reported as ignored, whichreads as "this machine can't run it" — so nothing has ever drawn attention to it.
The cause is two mappings that disagreed about what a feature name means.
Option parsing accepted these names through a deliberate work-around in
tools/render-test/options.cpp:The stated intent is the reasonable one: let the name through, and let the runtime decide. But the
runtime check used a different mapping,
_getFeatureFromName()inrender-test-main.cpp, builtonly from the
SLANG_RHI_FEATURESX-macro. None of these five names is in that list, so itreturned the sentinel
rhi::Feature::_Count, which was then pushed ontorequiredFeatures. And inslang-rhi:
_Countis never< _Count, sohasFeature()returns false unconditionally, on every device.render-test then returns
SLANG_E_NOT_AVAILABLEand the test is ignored — always, everywhere.The names were never valid:
grepping slang-rhi's entire git history forcooperative-matrix-reductionsand friends returns nothing. They correspond to the sub-features ofVK_NV_cooperative_matrix2(reductions, conversions, per-element ops, block loads, tensoraddressing), which slang-rhi exposes as the single feature
cooperative-matrix-2.2. Proposed solution
Give the mapping one owner.
getRenderFeatureFromName()becomes the single place that turns a-render-featurename into anrhi::Feature, and both callers use it:isValidFeatureName()is now justgetRenderFeatureFromName(name) != _Count;They can no longer disagree, because there is nothing left to disagree about.
Within that one function, the five sub-feature names resolve to
Feature::CooperativeMatrix2,since that is the feature slang-rhi actually reports for the extension providing them. A test gated
on
cooperative-matrix-reductionsnow runs on hardware implementingVK_NV_cooperative_matrix2and is skipped elsewhere — which is what the work-around's comment said should happen.
Removing the prefix escape hatch also restores loud failure for typos:
cooperative-matrix-fooisrejected at option-parse time again, instead of being accepted and silently disabling the test.
3. Change summary
tools/render-test/options.hgetRenderFeatureFromName(), with a comment on why one shared resolver existstools/render-test/options.cppSLANG_RHI_FEATUREStable);isValidFeatureName()reduced to a call to it; thestartsWith("cooperative-matrix-")work-around removedtools/render-test/render-test-main.cpp_getFeatureFromName(); the requirement check calls the shared resolver4. Concepts and vocabulary
rhi::Feature::_Count— the terminal enumerator of slang-rhi's feature enum, used here as a"not recognized" sentinel. It is not a feature, and
hasFeature(_Count)is false by construction.SLANG_RHI_FEATURES— the X-macro inslang-rhi.hpairing eachFeatureenumerator with itsstring name; both the old mappings were generated from it, which is why both were blind to any
name outside it.
VK_NV_cooperative_matrix2— the Vulkan extension providing the five sub-features named bythese tests. slang-rhi surfaces it as one feature,
cooperative-matrix-2.5. Process report
Is the input shape — an unrecognized feature name — valid, or should its producer be fixed?
Both, and the split matters. A name like
cooperative-matrix-reductionsappearing in a test is alegitimate thing for a test author to write: it names a real capability the test depends on. What
was not legitimate was resolving it to a sentinel and treating that as "device lacks support". So
the producer (the test files) is left untouched, and the consumer is fixed to resolve the name to
the feature that actually represents it. A name that resolves to nothing is genuinely
out-of-contract, and is now rejected loudly at parse time rather than silently degraded — matching
the project's "fail loudly on out-of-contract input" rule.
Why not add the five sub-features to slang-rhi instead? That is the better long-term shape and
is the real fix if per-sub-feature granularity is wanted: slang-rhi already reads
cooperativeMatrix2Featuresinvk-device.cppand could expose each boolean. But slang-rhi is aseparate repository and submodule, so it cannot land in this PR, and until it does these tests
remain dead. Resolving to
cooperative-matrix-2restores execution now and stays correctafterwards: if slang-rhi later exposes the granular features, the alias entries are deleted and the
generated table picks the names up with no other change. It is deliberately a mapping in one place,
not logic sprinkled across two.
Why
_Countas the sentinel is kept. The signature returns_Countfor "unknown" rather thanadding a new error path, because that is what the existing
_getFeatureFromName()already did andwhat
hasFeature()already tolerates. The change is that no caller now stores_Countinto arequirement list: parsing rejects such names before they reach it.
6. Verification
Mesa lavapipe 25.2.8 (implements neither
VK_KHR_cooperative_matrixnorVK_NV_cooperative_matrix2),Ubuntu 24.04, Release, clang-18.
-render-feature cooperative-matrix-reductionsCooperativeMatrix2; test ignored on lavapipe — correct, the device lacks it-render-feature cooperative-matrix-nonexistenterror 1006: invalid render feature name— previously accepted silently by the work-around-render-feature totally-bogus-featureerror 1006(unchanged behaviour, confirms no regression)map-element-single.slang,reduce.slangon lavapiperender-test+slang-testcompile cleanVK_NV_cooperative_matrix2,23 test invocations that have never run will begin running. They may fail — they have never been
validated against a real implementation, and nothing here can predict that. That is the point of
the change, but it means this PR should be landed with NVIDIA-hardware CI watched, and it is
reasonable to want those tests triaged before merge rather than after. No such hardware was
available in this environment.