[SPIR-V] Don't always force EXT_mesh_shader override

[rj123-nv]

• This changes the FeatureManager constructor, so that EXT_mesh_shader is still the default choice for mesh shaders when SPIR-V 1.4 or above is supported, but is not forced even when NV_mesh_shader is requested. Before, it was impossible to choose NV_mesh_shader in that case.

Fixes #6193

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Keenuts]

Hi @rj12122!
Thanks for the contribution!
Could you add some testing? You can see examples of such tests in:
DirectXShaderCompiler/tools/clang/test/CodeGenSPIRV/spv.inline.extension.hlsl or
DirectXShaderCompiler/tools/clang/test/CodeGenSPIRV/spv.inline.extension.unused.hlsl

To run them, ninja check-all (or ninja -C build check-all if you build is your directory`)
Thanks!

[s-perron]

A few existing tests are failing. I think they show that this might be more complicated because we consider all extension enabled by default.

[rj123-nv]

A few existing tests are failing. I think they show that this might be more complicated because we consider all extension enabled by default.

I think the problem is that EXT_mesh_shader isn't enabled by allowAllKnownExtensions (see FeatureManager::enabledByDefault(Extension ext))

So with my change, having no explicit extensions will call allowAllKnownExtensions(), which will enable NV_mesh_shader and won't enable EXT_mesh_shader.

I could save the value of opts.allowedExtensions.empty() into bool noExplicitExtensions before calling allowAllKnownExtensions(), then make the condition for enabling ext to
if (isTargetEnvSpirv1p4OrAbove() &&
noExplicitExtensions) {
allowExtension("SPV_EXT_mesh_shader");
}

Am I understanding right that if the user requests one extension, then they must request all extensions and can't rely on defaults? If not than what I just said wouldn't be right.

Let me know if you think of a better way, thanks

[rj123-nv]

Hi @rj12122! Thanks for the contribution! Could you add some testing? You can see examples of such tests in: DirectXShaderCompiler/tools/clang/test/CodeGenSPIRV/spv.inline.extension.hlsl or DirectXShaderCompiler/tools/clang/test/CodeGenSPIRV/spv.inline.extension.unused.hlsl

To run them, ninja check-all (or ninja -C build check-all if you build is your directory`) Thanks!

Sure, I'll add testing to the MR

[sudonatalie]

Am I understanding right that if the user requests one extension, then they must request all extensions and can't rely on defaults?

That's correct.

We don't enable any other extensions not explicitly specified by the user if -fspv-extension is set, and I don't see a strong reason SPV_EXT_mesh_shader should be an exception, so I'd like to propose that we remove this block of code from where it is since the fact that it was added out of line from where all other extension enabling is handled seems to be the source of this weirdness.

I think we should instead switch the FeatureManager::enabledByDefault's case Extension::EXT_mesh_shader: to return isTargetEnvSpirv1p4OrAbove(). This would also require moving the targetEnv initialization code to before the allowedExtensions setting code, but I think that's fine.

[rj123-nv]

@rj12122 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree [company="NVIDIA"]

[rj123-nv]

@rj12122 the command you issued was incorrect. Please try again.

Examples are:

@microsoft-github-policy-service agree

and

@microsoft-github-policy-service agree company="your company"

@microsoft-github-policy-service agree company="NVIDIA"

[rj123-nv]

I went with Natalie's suggestions in the current version

[Keenuts]

LGTM, thanks for adding the tests and making this change 😊

[rj123-nv]

Huh I removed -fcgl from the tests (and the other changes from Keenuts) and they work locally but I see there are failing tests on github

[Keenuts]

Yeah, that's on me, I gave you the fix only for one test, not for both. Here are the 2 bits to change to fix it.