Support specialization composite constants #211

Qining · 2016-03-21T20:17:05Z

Fix issue #163, support creation and usage of composite type
specialization constants.

e.g.:

layout(constant_id = 200) const float myfloat = 1.25;
layout(constant_id = 201) const int myint = 14;
struct structtype {
  float f;
  int i;
};
const structtype outer_struct_var = {myfloat, myint};
void main(){}

generated code (use glslangValidator):

// Module Version 10000
// Generated by (magic number): 80001
// Id's are bound by 12

                              Capability Shader
               1:             ExtInstImport  "GLSL.std.450"
                              MemoryModel Logical GLSL450
                              EntryPoint Vertex 4  "main"
                              Source GLSL 450
                              Name 4  "main"
                              Name 10  "structtype"
                              MemberName 10(structtype) 0  "f"
                              MemberName 10(structtype) 1  "i"
                              Decorate 7 SpecId 200
                              Decorate 9 SpecId 201
               2:             TypeVoid
               3:             TypeFunction 2
               6:             TypeFloat 32
               7:    6(float) SpecConstant 1067450368
               8:             TypeInt 32 1
               9:      8(int) SpecConstant 14
  10(structtype):             TypeStruct 6(float) 8(int)
              11:10(structtype) SpecConstantComposite 7 9
         4(main):           2 Function None 3
               5:             Label
                              Return
                              FunctionEnd

Rname two function names to match their functionalities.

Rename GlslangToSpvTraverser::createSpvSpecConstant() to
createSpvConstant();
Rename GlslangToSpvTraverser::createSpvConstant() to
createSpvConstantFromConstUnionArray()

Add function GlslangToSpvTraverser::createSpvConstantFromSubTree() to
handle constant creation from sub trees (e.g.: specialization constants).

Related PR: #208

Qining · 2016-03-21T20:18:50Z

@dekimir @dneto0 @johnkslang @AWoloszyn PTAL.

One thing I'm not sure is whether the tests cover enough cases.
And I will fix the format latter.

Qining · 2016-03-21T20:22:31Z

glslang/MachineIndependent/ParseHelper.cpp

@@ -484,7 +484,7 @@ TIntermTyped* TParseContext::handleBracketDereference(const TSourceLoc& loc, TIn
    TIntermTyped* result = nullptr;

    int indexValue = 0;
-    if (index->getQualifier().isConstant()) {
+    if (index->getQualifier().isFrontEndConstant()) {


calling isFrontEndConstant() means specialization constants won't get into the branch.
I think we do not need to check the index value for spec constants here, as we actually don't know their value, and checking based on their default values may not be very useful?

If we do need to check for the default value, that probably will be in another PR?

their default values may not be very useful

Default values should be useful.

That being said, I agree we can apply this change and postpone a bit.

Another point to consider (for a future PR):

merging together the subtree and the "constant union array" to one object, so that generally higher-level code like this doesn't care about that detail (this could also save memory, as they are mutually exclusive, and should be a union somewhere)

Probably will have a future PR for it :)

johnkslang · 2016-03-21T22:06:53Z

PTAL

Added a few comments. Other than that, it's looking good, on the right track.

One thing I'm not sure is whether the tests cover enough cases.

First goal is to support what existing CTS tests already want to do with GLSL. Second goal is providing 100% functionality. I suspect the latter will require fine-tuning KHR_vulkan_glsl a bit.

yavn · 2016-03-22T09:08:09Z

Hi @Qining, not sure if you had a chance to test this with spec constant tests in CTS (there's an open MR for them), but this fails on the following syntax:

layout(constant_id = 1) const float sc0 = 1.0;
struct Data {
    int  i;
    float sc;
    bool b;
};
const Data s0 = { 1, float(sc0), false };    // note the type cast

The culprit bit is the cast to float, which triggers an assertion SPIRV\SpvBuilder.cpp, line 792. If the cast isn't there, all is well. This happens only for scalar types.

Also nested initializer list syntax doesn't work with specialization constants:

struct Data {
    int  i;
    vec2 sc;
    bool b;
};
const Data s0 = { 1, { sc0, sc0 }, false };

This triggers an assertion here: initializer->getAsConstantUnion() || initializer->getType().getQualifier().isSpecConstant(), file .\MachineIndependent\ParseHelper.cpp, line 4944.
Note, I don't use braced initializer syntax in these tests, this one was just checked by me ad-hoc.

A great deal of tests passes with this change, so thank you for your work!

Qining · 2016-03-22T14:01:06Z

Hi @yavn, thanks for your test cases. I just checked these cases.

In the first case, float(sc0) generated an aggregate node, I will dive into it and try to fix it.
In the second case, it should be an issue about front-end propagation of SpecConstant, @johnkslang do you want to have a look, or I can dive into it and try to have a fix.

Actually the goal of this PR is to enable the spec constants tests in CTS. I will go ahead to test them.

Qining · 2016-03-22T18:22:48Z

Hi @yavn,

Please checkout the latest commit, the two failures should have been fixed by now.

I will go and take a look in the CTS tests.

Qining · 2016-03-22T18:31:41Z

glslang/MachineIndependent/ParseHelper.cpp

        } else
            return nullptr;
    }

    TIntermTyped* constructor = intermediate.setAggregateOperator(aggrNode, op, type, loc);
-    if (constructor->getType().getQualifier().isConstant() && specConst)
+    if (isConstConstrutor && hasSpecConst)


Hi @johnkslang , I think the constness of a constructor should depends on its components, or sequence if it is a aggregate type. And the variable (or say symbol?) declared with the constructor has its own type to determine if the AST node should be const or not, but its constructor (or say initializer)'s type should not be determined by the variable.

Please correct me if I'm wrong.
Determining the constructor's constness from its own sequence solved the following problem.

const user_define_struct s = {const_a, {spec_const_1, const_b}, const_c};

When we build up the inner struct, we don't have a variable whose storage is EvqConst, so the original code will not label the inner struct as SpecConst, but an temporary aggregate. This caused the second assertion failure mentioned in Yavn's post.

Yes, somewhere I discussed the issue of this being partly top down and partly bottom up.

Agreed that the spec.-constness aspect of a composite needs to be built bottom up. Unfortunately, other aspects of type in

const <type> <var> = <initializer>;

Come from <type>, not from the deduced type of <initializer>. It would be great to make it all bottom up, but perhaps off topic.

I assume it will suffice to get the correct bottom-up preservation of spec-constness in place, yes?

Yes, your last email help me a lot in understanding the code and find out where the issue is.

And I think it is sufficient, but I haven't tested with CTS tests.

yavn · 2016-03-23T09:16:50Z

Everything related to composite constants passes now. There is some syntax that still fails, but I don't think it's the topic of this pull request:

layout(constant_id = 1) gl_MaxImageUnits;    // specializing a built-in

const int a0[2 + sc0] = int[](1, 2, 3);      // array size expression
const int a1[sc0 + sc1] = int[](1, 2, 3);

Another issue is work group size specialization. It works, but I can't tell if it emits correct SPIR-V. I think it does, but my implementation doesn't handle it correctly. This might be a gray area in the spec.

Qining · 2016-03-23T13:52:52Z

Hi @yavn, I confirmed the result. It is the same on my machine.

I will file an issue in Vulkan repo about the a0[2 + sc0] = problem. The spec doesn't say how to behave in such a case.

The working group size specialization is not handled (and should be affected) by this CL. I can take a look of it later, probably should treat it the same as other aggregate variable?

Qining · 2016-03-23T14:06:25Z

Hi @yavn, another failure this CL will have with the CTS tests is the sc0 + sc1 operation. In this CL, binary or unary operations with spec constants and giving new spec constants are not yet implemented. So there suppose to be another failure upon 2 + sc0 and sc0 + sc1.

johnkslang · 2016-03-23T16:47:19Z

Note the specification says

Types containing arrays sized with a specialization constant cannot be compared, assigned as aggregates, or used in initializers.

So, along those lines, this should not work:

const int a1[sc0 + sc1] = int[](1, 2, 3);

But, perhaps the spec. can more explicitly list this kind of initializer (since it's not really an assignment, and it's the initializee that was specialized).

yavn · 2016-03-24T08:20:03Z

Oh, I didn't notice that... I'll update the tests to address this.

Qining · 2016-03-24T14:11:44Z

I've filed an issue in the Khronos gitlab vulkan/vulkan repo. Probably we should list more explicitly in the Spec about this case.

johnkslang · 2016-03-24T15:36:14Z

The point is that "per array element" code should not get generated (or input from GLSL), and have to change size, when a specialization constant changes value. It should be a fixed (constant) amount of "fix up" to change the size of a specialization constant. The list should be whatever that is.

Fix issue KhronosGroup#163, support creation and reference of composite type specialization constants. e.g.: ``` layout(constant_id = 200) const float myfloat = 1.25; layout(constant_id = 201) const int myint = 14; struct structtype { float f; int i; }; const structtype outer_struct_var = {myfloat, myint}; void main(){} ``` generated code (use glslangValidator): ``` // Module Version 10000 // Generated by (magic number): 80001 // Id's are bound by 12 Capability Shader 1: ExtInstImport "GLSL.std.450" MemoryModel Logical GLSL450 EntryPoint Vertex 4 "main" Source GLSL 450 Name 4 "main" Name 10 "structtype" MemberName 10(structtype) 0 "f" MemberName 10(structtype) 1 "i" Decorate 7 SpecId 200 Decorate 9 SpecId 201 2: TypeVoid 3: TypeFunction 2 6: TypeFloat 32 7: 6(float) SpecConstant 1067450368 8: TypeInt 32 1 9: 8(int) SpecConstant 14 10(structtype): TypeStruct 6(float) 8(int) 11:10(structtype) SpecConstantComposite 7 9 4(main): 2 Function None 3 5: Label Return FunctionEnd ``` Rname two function names to match their functionalities. 1) Rename `GlslangToSpvTraverser::createSpvSpecConstant()` to `createSpvConstant()`; 2) Rename `GlslangToSpvTraverser::createSpvConstant()` to `createSpvConstantFromConstUnionArray()` Add function `GlslangToSpvTraverser::createSpvConstantFromSubTree()` to handle constant creation from sub trees (e.g.: specialization constants). Related PR: KhronosGroup#208

johnkslang · 2016-03-25T19:14:53Z

SPIRV/GlslangToSpv.cpp

@@ -3851,6 +3854,66 @@ spv::Id TGlslangToSpvTraverser::createSpvConstant(const glslang::TType& glslangT
    return builder.makeCompositeConstant(typeId, spvConsts);
 }

+// Create constant ID from const initializer sub tree.
+spv::Id TGlslangToSpvTraverser::createSpvConstantFromConstSubTree(
+    const glslang::TIntermTyped* subTree) {


Have function open '{' start a new line.

Qining reviewed Mar 21, 2016
View reviewed changes

Qining force-pushed the spec-constants-composite branch from 3b3bdb1 to 1259a23 Compare March 21, 2016 23:10

Qining reviewed Mar 22, 2016
View reviewed changes

Qining mentioned this pull request Mar 24, 2016

WIP: Implement composite specialization constants #208

Closed

Qining force-pushed the spec-constants-composite branch from b07baac to 2c4c5ec Compare March 24, 2016 21:59

Qining force-pushed the spec-constants-composite branch from 2c4c5ec to 0840838 Compare March 24, 2016 22:04

johnkslang reviewed Mar 25, 2016
View reviewed changes

johnkslang merged commit c3869fe into KhronosGroup:master Mar 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support specialization composite constants #211

Support specialization composite constants #211

Qining commented Mar 21, 2016

Qining commented Mar 21, 2016

Qining Mar 21, 2016

johnkslang Mar 21, 2016

Qining Mar 21, 2016

johnkslang commented Mar 21, 2016

yavn commented Mar 22, 2016

Qining commented Mar 22, 2016

Qining commented Mar 22, 2016

Qining Mar 22, 2016

johnkslang Mar 22, 2016

Qining Mar 22, 2016

yavn commented Mar 23, 2016

Qining commented Mar 23, 2016

Qining commented Mar 23, 2016

johnkslang commented Mar 23, 2016

yavn commented Mar 24, 2016

Qining commented Mar 24, 2016

johnkslang commented Mar 24, 2016

johnkslang Mar 25, 2016

Support specialization composite constants #211

Support specialization composite constants #211

Conversation

Qining commented Mar 21, 2016

Qining commented Mar 21, 2016

Qining Mar 21, 2016

Choose a reason for hiding this comment

johnkslang Mar 21, 2016

Choose a reason for hiding this comment

Qining Mar 21, 2016

Choose a reason for hiding this comment

johnkslang commented Mar 21, 2016

yavn commented Mar 22, 2016

Qining commented Mar 22, 2016

Qining commented Mar 22, 2016

Qining Mar 22, 2016

Choose a reason for hiding this comment

johnkslang Mar 22, 2016

Choose a reason for hiding this comment

Qining Mar 22, 2016

Choose a reason for hiding this comment

yavn commented Mar 23, 2016

Qining commented Mar 23, 2016

Qining commented Mar 23, 2016

johnkslang commented Mar 23, 2016

yavn commented Mar 24, 2016

Qining commented Mar 24, 2016

johnkslang commented Mar 24, 2016

johnkslang Mar 25, 2016

Choose a reason for hiding this comment