Support for CapabilityShaderViewportIndex and CapabilityShaderLayer

[JustSid]

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 :)

[JustSid]

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]

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]

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

[JustSid]

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]

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]

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.

[CLAassistant]

All committers have signed the CLA.

[johnkslang]

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?

[JustSid]

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.

[johnkslang]

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.

[JustSid]

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 :)