Add ES 320 support and additional error checks for SPV_NV_mesh_shader #1507

sparmarNV · 2018-09-25T20:51:49Z

Add ES 320 support
Error out use of perprimitiveNV for non mesh/fragment shaders
Error out use of mesh/task shaders w/o use of NV_mesh_shader
Error out use of NV_mesh_shader for non task/mesh shaders
Error out use of perviewNV for non mesh shaders
Error out use of taskNV for non mesh/task shaders
Add test case for mesh shader with ES 320 profile

- Add ES 320 support - Error out use of perprimitiveNV for non mesh/fragment shaders - Error out use of mesh/task shaders w/o use of NV_mesh_shader - Error out use of NV_mesh_shader for non task/mesh shaders - Error out use of perviewNV for non mesh shaders - Error out use of taskNV for non mesh/task shaders - Add test case for mesh shader with ES 320 profile

sparmarNV · 2018-09-25T20:54:49Z

Adding @dgkoch @johnkslang for visibility

dgkoch · 2018-09-26T12:02:28Z

Test/spv.meshShaderUserDefined.mesh

@@ -1,4 +1,4 @@
-#version 450
+#version 320 es



Should we have a separate test shader for ES and GL profiles?

We already have 6 other tests with GL profiles. So we should be fine.

johnkslang · 2018-09-26T15:47:08Z

glslang/MachineIndependent/ParseHelper.cpp

@@ -4983,7 +4998,7 @@ void TParseContext::setLayoutQualifier(const TSourceLoc& loc, TPublicType& publi
        if (id.compare(0, 11, "local_size_") == 0) {
 #ifdef NV_EXTENSIONS
            if (language == EShLangMeshNV || language == EShLangTaskNV) {
-                profileRequires(loc, ~EEsProfile, 450, E_GL_NV_mesh_shader, "gl_WorkGroupSize");
+                requireExtensions(loc, 1, &E_GL_NV_mesh_shader, "gl_WorkGroupSize");


This looks like it dropped the 450 requirement(?).

You can call profileRequires() multiple times, once for each set of requirements:

profileRequires(loc, ~EEsProfile, ...); profileRequires(loc, EEsProfile, ...);

I added requireExtensions check which will ensure we have the extension.
The extension in turn has the profile GL450+/ES320+ requirement.
So I removed the redundant profileRequires() check from here.

Ah, that means it was a bug before(?), allowing 450 to support it without an extension, at least locally. If an extension must be present, the 450 should be a 0.

I think things are slightly flexible right when a brand new stage comes in, due to an extension, because their use is 100% correlated.

Yes, this was a bug before. I thought profileRequires would mandates both 450 && NV_mesh_shader. But since that is not the case, I had to change it to requireExtensions.

johnkslang · 2018-09-26T15:52:40Z

glslang/MachineIndependent/parseVersions.h

@@ -103,6 +103,9 @@ class TParseVersions {
    virtual void requireSpv(const TSourceLoc&, const char* op);
    virtual bool checkExtensionsRequested(const TSourceLoc&, int numExtensions, const char* const extensions[], const char* featureDesc);
    virtual void updateExtensionBehavior(const char* const extension, TExtensionBehavior);
+#ifdef NV_EXTENSIONS


I'm taking these ifdef out of this header.

johnkslang · 2018-09-26T16:03:05Z

glslang/MachineIndependent/Initialize.cpp

+            "\n");
+    }
+
+    if (profile != EEsProfile && version >= 450) {


The interface here is changing; if the test @dgkoch pointed out could easily be duplicated, it should be, unless you are sure all these interfaces changes are tested in the other files.

Okay, I will add another es 320 test.

johnkslang · 2018-09-26T16:05:06Z

glslang/MachineIndependent/ParseHelper.cpp

+            if ((profile == EEsProfile && version >= 320) ||
+                extensionTurnedOn(E_GL_NV_mesh_shader))
+                return;
+#endif


Shouldn't this case be fully self contained, and not rely on a fall through?

Yes, I forgot the break statement. Thanks for the catch.

johnkslang · 2018-09-26T16:07:27Z

glslang/MachineIndependent/Versions.cpp

@@ -834,6 +834,23 @@ void TParseVersions::updateExtensionBehavior(const char* extension, TExtensionBe
    }
 }

+#ifdef NV_EXTENSIONS
+// Validate if extension name is used with correct shader stage.
+bool TParseVersions::validateExtensionName(const TSourceLoc& loc, const char * const extension)


I don't think I agree with adding this function; it does not know what is going on.

There is a requireStage() function, to call from the place that does know what is going on. Does that work?

Or, maybe you want to call this one with a more specific name that says what it actually does, like check mesh stages.

Okay, I will inline the error check in Pp.cpp

I realized that we also need similar checks for other turing extensions.
So instead of inlining the error check, I am renaming the function to checkShaderStageForNVExtensions( )

johnkslang · 2018-09-26T16:08:21Z

glslang/MachineIndependent/parseVersions.h

@@ -103,6 +103,9 @@ class TParseVersions {
    virtual void requireSpv(const TSourceLoc&, const char* op);
    virtual bool checkExtensionsRequested(const TSourceLoc&, int numExtensions, const char* const extensions[], const char* featureDesc);
    virtual void updateExtensionBehavior(const char* const extension, TExtensionBehavior);
+#ifdef NV_EXTENSIONS


I am removing these #ifdefs from this header.

johnkslang · 2018-09-26T21:56:15Z

I think this failed from testing infrastructure problems, not glslang itself.

sparmarNV · 2018-09-26T21:59:36Z

I think this failed from testing infrastructure problems, not glslang itself.

Is it possible to rerun the tests?

johnkslang · 2018-09-26T22:00:25Z

glslang/MachineIndependent/preprocessor/Pp.cpp

@@ -874,6 +874,11 @@ int TPpContext::CPPextension(TPpToken* ppToken)
        return token;
    }

+#ifdef NV_EXTENSIONS
+    if (!parseContext.checkShaderStageForNVExtensions(ppToken->loc, extensionName))


Incorrect extension semantics are not preprocessor errors.

Maybe you want to do something in

void TParseVersions::updateExtensionBehavior(int line, const char* extension, const char* behaviorString)

johnkslang · 2018-09-26T22:05:33Z

glslang/MachineIndependent/Versions.cpp

@@ -834,6 +834,25 @@ void TParseVersions::updateExtensionBehavior(const char* extension, TExtensionBe
    }
 }

+#ifdef NV_EXTENSIONS
+// Check if extension is used with correct shader stage.
+bool TParseVersions::checkShaderStageForNVExtensions(const TSourceLoc& loc, const char * const extension)


Do we even need #extensions to turn on a stage that can only exist if the #extension is there? Or is this incremental functionality? The system already has to buy into the fact that the stage is present to even get this far, it seems a bit circular.

See also the stage logic recently added for these stages in ShaderLang.cpp around line 568:

// Correct for stage type... switch (stage) {

This is to error out the case where an extension is incorrectly used in some other shader stage.
For e.g. using GL_ARB_compute_shader extension inside a non-compute shader.
I believe glslang doesn't error out such use cases.
I agree I should move the error check in updateExtensionBehavior( ).

Okay, I understand. I keep thinking it will disappear. But, it's still here, so more issues:

it should be not be called an NV function

it should not do odd checks for error reporting; it can allow errors to continue

basically, it only needs to be the one if-test and call to requireStage

(implying this is a void, not bool)

It would be fine with me if that one statement was inlined and the rest disappeared. But, to keep the function is also okay, for all checks like this, in which case it won't be NV-specific and should only do the checking.

Something like:

void TParseVersions::checkExtensionStage(const TSourceLoc& loc, const char * const extension) { // GL_NV_mesh_shader extension is only allowed in task/mesh shaders if (strcmp(extension, "GL_NV_mesh_shader") == 0) requireStage(loc, (EShLanguageMask)(EShLangTaskNVMask | EShLangMeshNVMask), "#extension GL_NV_mesh_shader"); }

More background: there is already a generic mechanism for stopping parsing around the first error (unless cascading errors are requested), so individual error checks do not need to worry about termination, unless the next lines of code would crash glslang.

Thanks for the detailed feedback.
I have done the changes to the function as suggested above.
Only thing which I added was the #ifdef NV_EXTENSIONS in function body, because the underlying code used EShLang*NVMask which is also guarded by same #ifdef.

johnkslang · 2018-09-26T22:10:05Z

I think the preprocessor stuff needs fixing anyway; I don't think anything here should be changing the preprocessor. GLSL proper has all the semantic helpers needed; per my recent comments.

…ehavior()

…nsionStage Also continue error checking post requireStage() check

sparmarNV · 2018-09-27T18:16:03Z

Also let me know if you want me to squash all commits into 1.

johnkslang · 2018-09-28T12:25:01Z

Yes, squashing would be better; I saw that too late. Next time.

dgkoch reviewed Sep 26, 2018

View reviewed changes

johnkslang reviewed Sep 26, 2018

View reviewed changes

changes based on feedback

3ae0f58

johnkslang reviewed Sep 26, 2018

View reviewed changes

sparmarNV added 2 commits September 26, 2018 16:12

Call checkShaderStageForNVExtensions() from function updateExtensionB…

251344f

…ehavior()

Based on feedback rename checkShaderStageForNVExtensions to checkExte…

80b9872

…nsionStage Also continue error checking post requireStage() check

johnkslang merged commit 4508a81 into KhronosGroup:master Sep 28, 2018

sparmarNV deleted the fix-SPV_NV_mesh_shader branch October 2, 2018 21:09

zeux mentioned this pull request Oct 22, 2018

Using perprimitiveNV in fragment shader produces a validation error requiring SPV_NV_mesh_shader KhronosGroup/Vulkan-ValidationLayers#414

Closed

johnkslang mentioned this pull request Oct 25, 2018

Fragment shader with perprimitiveNV doesn't have OpExtension SPV_NV_mesh_shader #1541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ES 320 support and additional error checks for SPV_NV_mesh_shader #1507

Add ES 320 support and additional error checks for SPV_NV_mesh_shader #1507

sparmarNV commented Sep 25, 2018

sparmarNV commented Sep 25, 2018

dgkoch Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang commented Sep 26, 2018

sparmarNV commented Sep 26, 2018

johnkslang Sep 26, 2018

johnkslang Sep 26, 2018

johnkslang Sep 26, 2018

johnkslang Sep 26, 2018

sparmarNV Sep 26, 2018

johnkslang Sep 27, 2018

johnkslang Sep 27, 2018 •

edited

Loading

johnkslang Sep 27, 2018

sparmarNV Sep 27, 2018

johnkslang commented Sep 26, 2018

sparmarNV commented Sep 27, 2018

johnkslang commented Sep 28, 2018

Add ES 320 support and additional error checks for SPV_NV_mesh_shader #1507

Add ES 320 support and additional error checks for SPV_NV_mesh_shader #1507

Conversation

sparmarNV commented Sep 25, 2018

sparmarNV commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkslang commented Sep 26, 2018

sparmarNV commented Sep 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkslang Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkslang commented Sep 26, 2018

sparmarNV commented Sep 27, 2018

johnkslang commented Sep 28, 2018

johnkslang Sep 27, 2018 •

edited

Loading