Support for CapabilityShaderViewportIndex and CapabilityShaderLayer by JustSid · Pull Request #2432 · KhronosGroup/glslang

JustSid · 2020-10-22T22:39:19Z

With SPIR-V 1.5, CapabilityShaderViewportIndexLayerEXT has gone the way of the dodo and has been replaced with CapabilityShaderViewportIndex and CapabilityShaderLayer. These two make gl_Layer and gl_ViewportIndex actual built ins, that don't require any extension anymore. Unfortunately glslang seems to have no idea about this, using these native built ins when targeting SPIR-V 1.5 will result in an error that extensions are required to make them work.

This pull request is split into two commits. The first is basic emitting of CapabilityShaderViewportIndex and CapabilityShaderLayer when encountering the new built ins under SPIR-V 1.5, as well as no longer requiring an extension to use the built ins. The second commit expands on this by introducing a fallback to CapabilityShaderViewportIndexLayerEXT when one of the previous extensions is present as a source extension. So shaders that use eg. GL_ARB_shader_viewport_layer_array right now won't get opted into the new built ins and capability.

The second commit required adding helper functions to the Builder object. Not sure if that's the correct way to do that, or if there is a better way. I'm new to the code base, so I'm not sure what the preferred way for this kind of thing is. Let me know if there are any problems and I'd be happy to correct them :)

…ity ShaderViewportIndex and using gl_Layer will emit OpCapability CapabilityShaderLayer. OpCapability ShaderViewportIndexLayerEXT will only get emitted if the target < SPIR-V 1.5

…pCapability ShaderViewportIndexLayerEXT, even when targeting SPIR-V 1.5

JustSid · 2020-10-22T22:58:40Z

I have also made a pull request for spirv-val that fixes a false positive when validating new shaders. That pull request can be found here: KhronosGroup/SPIRV-Tools#3986

I'm guessing this pull request won't be merge-able until the other one is merged and the external dependencies can be updated.

johnkslang · 2020-10-27T17:49:45Z

With SPIR-V 1.5, CapabilityShaderViewportIndexLayerEXT has gone the way of the dodo

This isn't quite true. ShaderViewportIndexLayer* is still in the spec., not deprecated, still reserved for extension use.

and has been replaced with CapabilityShaderViewportIndex and CapabilityShaderLayer.

It was split into these two finer-granularity capabilities.

These two make gl_Layer and gl_ViewportIndex actual built ins

They've always been built-ins.

that don't require any extension anymore.

Yes, I think that's the point: SPIR-V 1.5 brought this functionality into core, using different capabilities than it had when using the extension.

When targeting 1.5, glslang should use the newer-core capabilities, and before that it should us the extension capabilities. Both should still be in play.

This should be triggered by seeing use/declaration, not by source extension, and when triggered it should emit capabilities/extensions based on the target, not on source extension.

johnkslang · 2020-10-27T17:53:28Z

SPIRV/SpvBuilder.h


    unsigned int getSpvVersion() const { return spvVersion; }

+    bool hasSourceExtension(const char* ext) const


We should not be making these decisions based on source extension, only on feature use and target environment.

JustSid · 2020-10-27T18:09:26Z

This isn't quite true. ShaderViewportIndexLayer* is still in the spec., not deprecated, still reserved for extension use.

Huh, TIL, thank you. I'm no spec lawyer, I just went along the ride with compiler errors/validation layer problems until I made it here.

Although from how I understand this, glslang is currently emitting invalid SPIR-V because the compiler won't emit the OpExtension requirement for ShaderViewportIndexLayerEXT (SPV_EXT_shader_viewport_index_layer) nor will it emit the new capabilities that don't require an extension when targeting SPIR-V 1.5.

This should be triggered by seeing use/declaration, not by source extension, and when triggered it should emit capabilities/extensions based on the target, not on source extension.

Can you elaborate on what you think this should look like? I'm happy to make any changes to this pull request so I can target SPIR-V 1.5 and get valid SPIR-V out of it. It's my understanding that ShaderViewportIndexLayerEXT is inherently tied to VK_EXT_shader_viewport_index_layer and GL_ARB_shader_viewport_layer_array (and friends, but for simplicity sake let's pretend there's only one extension here). So if you target SPIR-V 1.5 and want to use the new Vulkan 1.2 core features without relying on extensions, the compiler has to emit the new capabilities. On the other hand, if you want to use ShaderViewportIndexLayerEXT you also have to enable VK_EXT_shader_viewport_index_layer, so if the compiler emits that your Vulkan 1.2 application is malformed if you don't enable that extension.

This led me to using the source extension as a hint what to do. The Vulkan spec text is a bit vague (again, no spec lawyer), but it says you get VK_EXT_shader_viewport_index_layer if you enable the Vulkan 1.2 shaderOutputViewportIndex and shaderOutputLayer features, but it doesn't say if enabling just VK_EXT_shader_viewport_index_layer gives you the same as enabling both features or if it just gives you SPV_EXT_shader_viewport_index_layer support. If it's the latter, then there should be a way for the shader author to get the compiler to emit what they need. If they are both synonymous it doesn't really matter what the compiler does as long as it's valid.

johnkslang · 2020-10-27T19:03:42Z

Yes, I think glslang needs a fix, to reflect the split into two capabilities, based on what the target version is.

I think we need something like:

// for Layer
if (SPIR-V is going to emit use or declaration for Layer)
  if (target version is 1.5 or above)
    addCapability(ShaderLayer)
  else
    addExtension(SPV_EXT_shader_viewport_index)
    addCapabilitiy(ShaderViewportIndexLayerEXT)

Similarly for ViewportIndex.

Doing it from the top of my head, initial stab. Does this miss something?

JustSid · 2020-10-27T19:08:22Z

Ah, gotcha. I believe this was already implemented by my first commit (670536b). I can revert the second commit and just leave the first one.

…ack to OpCapability ShaderViewportIndexLayerEXT, even when targeting SPIR-V 1.5" This reverts commit dccca82.

CLAassistant · 2020-10-28T18:09:07Z

All committers have signed the CLA.

johnkslang · 2020-10-28T21:30:20Z

glslang/MachineIndependent/Initialize.cpp

        BuiltInVariable("gl_ViewportIndex",   EbvViewportIndex,  symbolTable);

-        if (language != EShLangGeometry) {
+        if (language != EShLangGeometry && spvVersion.spv < glslang::EShTargetSpv_1_5) {


These variables should have the same GLSL semantics regardless of selected SPIR-V target.

Or, did the SPIR-V extension really say that core GLSL is modified to not need the GLSL extension?

Errr, I'm no spec lawyer and I might be completely wrong here, but how I read this is as follows:

SPV_EXT_shader_viewport_index_layer makes the layer and viewport index built ins available outside of the geometry shaders. To quote the extension text:

The new ShaderViewportIndexLayerEXT capability allows the Layer and ViewportIndex builtin variables to be exported from Vertex or Tessellation shaders, in addition to Geometry shaders. This is functionality added GLSL by both GL_ARB_shader_viewport_layer_array and GL_NV_viewport_array2, and separately by GL_AMD_vertex_shader_layer, and GL_AMD_vertex_shader_viewport_index.

SPIR-V 1.5 adds the following to the built ins:

ViewportIndex
Viewport selection for viewport transformation when using multiple viewports. See the client API specification for more detail.
[...]
The ShaderViewportIndex capability allows for a ViewportIndex output by a Vertex or Tessellation Execution Model.

If I read this right, and mind you, big if here, then that means that targeting SPIR-V 1.5 means that ViewportIndex built in can just be used without anything else. Same goes for the layer.

Now, this doesn't necessarily mean that the GLSL gl_Layer and gl_ViewportIndex tokens are usable without extension. I kinda assumed they are, but in my mind GLSL is just the human readable form of SPIR-V and so my assumption might be completely off.

So, the spec. quotes are about SPIR-V and not about GLSL. SPIR-V and GLSL have independent specifications, and it is not required that their extension declarations run in parallel.

Glslang is currently representing pretty detailed versioning about these variables, and I'd like to not change that without seeing the spec. language for it.

I think the SPIR-V part of your change is good though.

Alright, that makes sense. Sorry, the GLSL/SPIR-V/Vulkan line is blurry at best to me.

I have reverted the piece of code in question. Thank you for looking at this :)

…age still requires one of the viewport extensions even when targeting SPIR-V 1.5 (Fixes a problem introduced by 670536b)

JustSid added 2 commits October 22, 2020 15:04

When targeting SPIR-V 1.5, using gl_ViewportIndex will emit OpCapabil…

670536b

…ity ShaderViewportIndex and using gl_Layer will emit OpCapability CapabilityShaderLayer. OpCapability ShaderViewportIndexLayerEXT will only get emitted if the target < SPIR-V 1.5

When using one of the viewport/layer arrays extensions, fallback to O…

dccca82

…pCapability ShaderViewportIndexLayerEXT, even when targeting SPIR-V 1.5

JustSid mentioned this pull request Oct 22, 2020

spirv-val: Allow the ViewportIndex and Layer built-ins on SPIR-V 1.5 KhronosGroup/SPIRV-Tools#3986

Merged

johnkslang added the kokoro:run Trigger Google bot runs label Oct 27, 2020

kokoro-team removed the kokoro:run Trigger Google bot runs label Oct 27, 2020

johnkslang reviewed Oct 27, 2020

View reviewed changes

Revert "When using one of the viewport/layer arrays extensions, fallb…

f5cff7e

…ack to OpCapability ShaderViewportIndexLayerEXT, even when targeting SPIR-V 1.5" This reverts commit dccca82.

johnkslang reviewed Oct 28, 2020

View reviewed changes

Using gl_Layer and gl_ViewportIndex outside of the geometry shader st…

9e9ac05

…age still requires one of the viewport extensions even when targeting SPIR-V 1.5 (Fixes a problem introduced by 670536b)

johnkslang merged commit 56350ca into KhronosGroup:master Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for CapabilityShaderViewportIndex and CapabilityShaderLayer#2432

Support for CapabilityShaderViewportIndex and CapabilityShaderLayer#2432
johnkslang merged 4 commits intoKhronosGroup:masterfrom
JustSid:master

JustSid commented Oct 22, 2020

Uh oh!

JustSid commented Oct 22, 2020

Uh oh!

johnkslang commented Oct 27, 2020

Uh oh!

johnkslang Oct 27, 2020

Uh oh!

JustSid commented Oct 27, 2020

Uh oh!

johnkslang commented Oct 27, 2020

Uh oh!

JustSid commented Oct 27, 2020

Uh oh!

CLAassistant commented Oct 28, 2020 •

edited

Loading

Uh oh!

johnkslang Oct 28, 2020

Uh oh!

JustSid Oct 28, 2020 •

edited

Loading

Uh oh!

johnkslang Oct 29, 2020

Uh oh!

JustSid Oct 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		unsigned int getSpvVersion() const { return spvVersion; }

		bool hasSourceExtension(const char* ext) const

Conversation

JustSid commented Oct 22, 2020

Uh oh!

JustSid commented Oct 22, 2020

Uh oh!

johnkslang commented Oct 27, 2020

Uh oh!

johnkslang Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

JustSid commented Oct 27, 2020

Uh oh!

johnkslang commented Oct 27, 2020

Uh oh!

JustSid commented Oct 27, 2020

Uh oh!

CLAassistant commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnkslang Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

JustSid Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnkslang Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

JustSid Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Oct 28, 2020 •

edited

Loading

JustSid Oct 28, 2020 •

edited

Loading