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

Validator should not check for capability when seeing decorations of ClipDistance, CullDistance, or PointSize #365

Closed
chrisforbes opened this issue Aug 22, 2016 · 10 comments
Assignees

Comments

@chrisforbes
Copy link
Contributor

See KhronosGroup/Vulkan-LoaderAndValidationLayers#858, KhronosGroup/glslang#472.

@dneto0 dneto0 self-assigned this Aug 23, 2016
@johnkslang johnkslang changed the title Validator needs to be relaxed to only require capabilities for decorated objects that are statically used Validator should not check for capability when seeing decorations of ClipDistance, CullDistance, or PointSize Aug 23, 2016
@johnkslang
Copy link
Member

I fixed the title, as this should not be generalized beyond ClipDistance, CullDistance, and PointSize.

@dneto0
Copy link
Collaborator

dneto0 commented Aug 23, 2016

I made that change to the validator on July 7:
06581f5

But it's conditioned on using a Vulkan 1.0 context, since it was a Vulkan decision to relax that rule.
@chrisforbes Please confirm that you have code at least as fresh as that.

The master branch of the Vulkan layers tree specifies revision 6c61bf2 from July 22 which should be fresh enough. And its only call to spvContextCreate does use SPV_ENV_VULKAN_1_0.

@dneto0
Copy link
Collaborator

dneto0 commented Aug 23, 2016

Commented on the layers bug suggesting that maybe the command line tool was not being used, and that you should use --target-env vulkan1.0 option.

@johnkslang
Copy link
Member

What situation should error on ClipDistance (et. al.) Decoration with no capability? I don't think there is one; rather the validator should unconditionally tolerate ClipDistance/CullDistance/PointSize decoration, and not require specific options be specified to do so.

@dneto0
Copy link
Collaborator

dneto0 commented Aug 23, 2016

The basic rule applied is any mention of an enum that lists a required capability may only be in a module that declares that capability. That's even as a decoration.
The specific rule that overrides that default is in the Vulkan env spec, which is why the relaxation of the check is only done for Vulkan 1.0 targets.

In particular we now have OpenGL environments in SPIRV-Tools. Almost certainly the same rule override would apply there. Since it's basically the same GLSL variation is going to be used any time ClipDistance (and friends) is to be used, there's a strong argument to say the rule should always be relaxed for ClipDistance, CullDistance, PointSize. I would guess that's your recommendation @johnkslang ?

@johnkslang
Copy link
Member

Yes, the exception is driven by whether it is ClipDistance/CullDistance/PointSize, and not driven by what the environment is.

The general rule (decoration requires capability), would also apply universally, unless it was ClipDistance/CullDistance/PointSize.

So, slice along decoration name, not along environment.

@dneto0
Copy link
Collaborator

dneto0 commented Aug 23, 2016

Ok. I'll do that.

@dneto0
Copy link
Collaborator

dneto0 commented Aug 23, 2016

FYI. The previous relaxation was prompted by #261

@dneto0
Copy link
Collaborator

dneto0 commented Aug 23, 2016

FYI. Mention of PointSize as a BuiltIn will still trigger requirement of the Shader capability. Nobody has complained about that yet.

@johnkslang
Copy link
Member

johnkslang commented Aug 23, 2016

So, we need to reopen? That's not to spec.

Note this is all "spirit of the law" as well as now to spec.:

  • the point of capability is to declare before utilizing a platform's feature
  • decoration does not itself utilize the platform's feature
  • ClipDistance/CullDistance/PointSize are special because of cross-linkage rules, in that they may need decoration without subsequent use

@johnkslang johnkslang reopened this Aug 23, 2016
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 24, 2016
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 24, 2016
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 24, 2016
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants