Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPV: Vulkan: inconsistent gl_PerVertex struct generated, depending on source language version. Not following KHR_vulkan_glsl advice #334

Closed
dneto0 opened this issue Jun 8, 2016 · 5 comments

Comments

@dneto0
Copy link
Contributor

dneto0 commented Jun 8, 2016

KHR_vulkan_glsl says that an implicitly defined in/out gl_PerVertex should be structured (in a non-fragment shader) as follows:

          in/out gl_PerVertex {
                gl_Position
                gl_PointSize
                gl_ClipDistance
                gl_CullDistance
            }        

But the structure emitted depends on the source language version.
For example, a vertex shader that just uses gl_Position will get a struct with just Position and PointSize in 310es.

@yavn
Copy link

yavn commented Jun 14, 2016

This, coupled with recent changes to ClipDistance capability, is giving me some grief in CTS. Shaders that use gl_Position are now failing SPIR-V validation.

Is it allowed to emit a gl_PerVertex block with only used members, e.g. only gl_Position?

Otherwise I'm facing this problem:

  • shader (I used GLSL version 440) writes to gl_Position
  • glslang generates gl_PerVertex with gl_Position, gl_PointSize, and gl_ClipDistance
  • members are annotated with BuiltIn
  • but ClipDistance capability is not used
  • as a result, SPIR-V is invalid

Shader is deemed invalid by both spirv-tools and validation layers. Are they wrong, or is glslang doing something not quite right?

For me it kind of makes sense to not require a capability if we don't write to the variable. On the other hand SPIR-V spec uses language:

ClipDistance
Uses the ClipDistance BuiltIn.

So apparently the mere act of using a BuiltIn (declaring it?) is sufficient to require the capability.

@dneto0
Copy link
Contributor Author

dneto0 commented Jun 15, 2016

@yavn Yes, we've noticed the same thing. Your suggestion is very reasonable and would entail an update to the Vulkan environment spec.

In the meantime, a workaround is to have your shader explicitly redeclare gl_PerVertex to only have the members it uses, e.g.:

out gl_PerVertex {
  vec4 gl_Position;
};

@yavn
Copy link

yavn commented Jun 15, 2016

Thanks, that's a good idea! I forgot about this possibility.

@johnkslang
Copy link
Member

KHR_vulkan_glsl says that an implicitly defined

I don't think the intent was to limit this to just implicitly defined, but I also don't think that works either.

a workaround is to have your shader explicitly redeclare gl_PerVertex to only have the members it uses

Right.

SPIR-V itself is must always be explicit, so if it supports the latter of these quotes (to subset the four members based on explicit GLSL declaration), then it can support subsetting for implicit too.

So, in a way, any implication that SPIR-V must be the same (all four present) for GLSL implicit is inconsistent with any implication that SPIR-V must be flexible (different subsets present) for GLSL explicit declaration.

So, one way or another the spec should be fixed here, and if done, it is likely that glslang is doing something okay by subsetting.

The overall intent to obey is this: any complexities in implicit vs. explicit and policies choosing subsetting are to be isolated to high-level languages and their front ends, while SPIR-V itself remains always explicit and without any "HLL per-version" rules that pervaded GLSL.

@johnkslang
Copy link
Member

This was fixed long ago, but connection here was not made. GL_KHR_vulkan_glsl currently says:

            in/out gl_PerVertex {   // some subset of these members will be used
                gl_Position
                gl_PointSize
                gl_ClipDistance
                gl_CullDistance
            } ...

      ...
      The subset and order of members will match between stages sharing an
      interface.

This directly speaks to the original claim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants