Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 34 additions & 24 deletions tools/render-test/options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,40 +17,50 @@ namespace renderer_test
{
using namespace Slang;

// Helper function to check if a feature name is valid
static bool isValidFeatureName(
const UnownedStringSlice& featureName,
DiagnosticSink* sink,
SourceLoc loc)
rhi::Feature getRenderFeatureFromName(const UnownedStringSlice& featureName)
{
// WAR: Accept cooperative-matrix-2 sub-features until RHI backend supports them
// These features will be gracefully skipped at runtime if hardware doesn't support them
if (featureName.startsWith("cooperative-matrix-"))
// slang-rhi reports VK_NV_cooperative_matrix2 as one feature, but tests name the individual
// sub-features the extension provides. Resolve each of them to that single feature so a test
// gated on a sub-feature runs on hardware that supports the extension, rather than being
// skipped everywhere.
static const UnownedStringSlice kCooperativeMatrix2SubFeatures[] = {
UnownedStringSlice::fromLiteral("cooperative-matrix-block-loads"),
UnownedStringSlice::fromLiteral("cooperative-matrix-conversions"),
UnownedStringSlice::fromLiteral("cooperative-matrix-per-element-operations"),
UnownedStringSlice::fromLiteral("cooperative-matrix-reductions"),
UnownedStringSlice::fromLiteral("cooperative-matrix-tensor-addressing"),
};
for (const auto& subFeature : kCooperativeMatrix2SubFeatures)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

{
if (sink)
if (featureName == subFeature)
{
sink->diagnoseRaw(
Severity::Warning,
"Using cooperative-matrix-2 feature that is not yet fully supported "
"in RHI backend. "
"Test will be skipped if hardware doesn't support it.");
return rhi::Feature::CooperativeMatrix2;
}
return true;
}
#define SLANG_RHI_FEATURES_X(id, name) name,
static const char* kValidFeatureNames[] = {SLANG_RHI_FEATURES(SLANG_RHI_FEATURES_X)};
#undef SLANG_RHI_FEATURES_X

static const int kFeatureCount = sizeof(kValidFeatureNames) / sizeof(kValidFeatureNames[0]);
#define SLANG_RHI_FEATURES_X(id, name) {UnownedStringSlice::fromLiteral(name), rhi::Feature::id},
struct FeatureNameMapEntry
{
UnownedStringSlice name;
rhi::Feature feature;
};
static const FeatureNameMapEntry kFeatureNameMap[] = {SLANG_RHI_FEATURES(SLANG_RHI_FEATURES_X)};
#undef SLANG_RHI_FEATURES_X

for (int i = 0; i < kFeatureCount; i++)
for (const auto& entry : kFeatureNameMap)
{
if (featureName == UnownedStringSlice(kValidFeatureNames[i]))
if (featureName == entry.name)
{
return true;
return entry.feature;
}
}
return false;
return rhi::Feature::_Count;
}

/// Return true if `featureName` names a feature the runtime requirement check can evaluate.
static bool isValidFeatureName(const UnownedStringSlice& featureName)
{
return getRenderFeatureFromName(featureName) != rhi::Feature::_Count;
}

static rhi::DeviceType _toRenderType(Slang::RenderApiType apiType)
Expand Down Expand Up @@ -162,7 +172,7 @@ static rhi::DeviceType _toRenderType(Slang::RenderApiType apiType)
for (const auto& value : values)
{
// Validate that the feature name is recognized
if (!isValidFeatureName(value, &sink, featuresArg.loc))
if (!isValidFeatureName(value))
{
sink.diagnose(
featuresArg.loc,
Expand Down
11 changes: 11 additions & 0 deletions tools/render-test/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,15 @@ struct Options
Options& outOptions);
};

/// 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);
Comment on lines +117 to +126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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


} // namespace renderer_test
21 changes: 1 addition & 20 deletions tools/render-test/render-test-main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,25 +193,6 @@ static void _outputProfileTime(uint64_t startTicks, uint64_t endTicks)
out.print("profile-time=%g\n", time);
}

static rhi::Feature _getFeatureFromName(const UnownedStringSlice& featureName)
{
struct FeatureNameMapEntry
{
const char* name;
rhi::Feature feature;
};

#define SLANG_RHI_FEATURES_X(id, name) {name, rhi::Feature::id},
static const FeatureNameMapEntry kFeatureNameMap[] = {SLANG_RHI_FEATURES(SLANG_RHI_FEATURES_X)};
#undef SLANG_RHI_FEATURES_X

for (auto& entry : kFeatureNameMap)
if (featureName == UnownedStringSlice(entry.name))
return entry.feature;

return rhi::Feature::_Count;
}

class ProgramVars;

struct ShaderOutputPlan
Expand Down Expand Up @@ -1918,7 +1899,7 @@ static SlangResult _innerMain(

List<rhi::Feature> requiredFeatureList;
for (auto& name : options.renderFeatures)
requiredFeatureList.add(_getFeatureFromName(name.getUnownedSlice()));
requiredFeatureList.add(getRenderFeatureFromName(name.getUnownedSlice()));

desc.requiredFeatures = requiredFeatureList.getBuffer();
desc.requiredFeatureCount = (int)requiredFeatureList.getCount();
Expand Down
Loading