bool is converted to uint32

[amdrexu]

Hi,

I have this shader:

#version 450

layout(binding = 0) uniform Uniforms
{
    bvec4 b4;
};

out vec4 fragColor;

void main()
{
    fragColor = (b4.x && b4.y) ? vec4(1.0) : vec4(0.5);
}

The output SPIR-V tokens have these two instructions:

24:     13(int) CompositeExtract 23 0
                SelectionMerge 26 None
                BranchConditional 24 25 26

30:    12(bool) Phi 24 5 29 25

From the SPIR-V, we have this limitation:

OpBranchConditional:
Condition must be a Boolean type scalar.

OpPhi:
All Variables must have a type matching Result Type.

Also, OpAny (use any()) also has this problem because the source operand is uint32 instead of bool now, which violates the SPIR-V spec.

I know there is a change (103bef9) converting bool to uint32 implicitly (if the bool variable is defined in block). It is OK to have a uint32 variable loaded from the block. But I think a bitcast instruction should be inserted before using this uint32 variable in all instructions which expect a bool variable (based on SPIR-V spec). Otherwise, this is confusing and makes the output SPIR-V tokens look inconsistent.

Thank you for double checking it.

[amdrexu]

Today, I find this issue is ubiquitous.

See this fragment shader:

layout(binding = 0, std430) buffer Buffer
{
    bvec3 b3;
};

void main()
{
    b3 = bvec3(1.0);
}

We have such SPIR-V tokens;

                          Capability Shader
           1:             ExtInstImport  "GLSL.std.450"
                          MemoryModel Logical GLSL450
                          EntryPoint Fragment 4  "main"
                          ExecutionMode 4 OriginUpperLeft
                          Source GLSL 450
                          Name 4  "main"
                          Name 8  "Buffer"
                          MemberName 8(Buffer) 0  "b3"
                          Name 10  ""
                          MemberDecorate 8(Buffer) 0 Offset 0
                          Decorate 8(Buffer) BufferBlock
                          Decorate 10 DescriptorSet 0
                          Decorate 10 Binding 0
           2:             TypeVoid
           3:             TypeFunction 2
           6:             TypeInt 32 0
           7:             TypeVector 6(int) 3
   8(Buffer):             TypeStruct 7(ivec3)
           9:             TypePointer Uniform 8(Buffer)
          10:      9(ptr) Variable Uniform
          11:             TypeInt 32 1
          12:     11(int) Constant 0
          13:             TypeBool
          14:             TypeVector 13(bool) 3
          15:    13(bool) ConstantTrue
          16:   14(bvec3) ConstantComposite 15 15 15
          17:             TypePointer Uniform 7(ivec3)
     4(main):           2 Function None 3
           5:             Label
          18:     17(ptr) AccessChain 10 12
                          Store 18 16
                          Return
                          FunctionEnd

The instruction "Store 18 16" violates the SPIR-V spec, which requires the destination and source operand should have the same type.

[amdrexu]

I have made a fix in 2725323. Please have a review.

[gfxstrand]

This bug actually goes a bit deeper. Any time you have a bvecN in a UBO/SSBO, the code that's supposed to insert the int -> bool conversion simply doesn't.

Also, I think that just making them integers is somewhat bad policy. What happens if you have do a full struct copy out of a UBO and that struct happens to have booleans in it. What do you do with that? Do you have to emit the load from the UBO one basic type at a time? Do you have to do extract operations to get to the booleans, convert them, and then build the struct back up? It's pretty trivial for the driver to insert the int -> bool conversion as it does the load but as of the commit in question, it no longer has the information.

[amdrexu]

Yes, you are definitely right. I have this shader showing the problem you mentioned.

#version 450

struct Data
{
    bvec2 b2;
};

layout(std140, binding = 0) buffer Buffer
{
    Data data;
};

void main()
{
    Data d = data;
    gl_Position = d.b2.x ? vec4(0.0) : vec4(1.0);
}

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

                              Capability Shader
                              Capability ClipDistance
                              Capability CullDistance
               1:             ExtInstImport  "GLSL.std.450"
                              MemoryModel Logical GLSL450
                              EntryPoint Vertex 4  "main" 28
                              Source GLSL 450
                              Name 4  "main"
                              Name 8  "Data"
                              MemberName 8(Data) 0  "b2"
                              Name 10  "d"
                              Name 13  "Data"
                              MemberName 13(Data) 0  "b2"
                              Name 14  "Buffer"
                              MemberName 14(Buffer) 0  "data"
                              Name 16  ""
                              Name 26  "gl_PerVertex"
                              MemberName 26(gl_PerVertex) 0  "gl_Position"
                              MemberName 26(gl_PerVertex) 1  "gl_PointSize"
                              MemberName 26(gl_PerVertex) 2  "gl_ClipDistance"
                              MemberName 26(gl_PerVertex) 3  "gl_CullDistance"
                              Name 28  ""
                              MemberDecorate 13(Data) 0 Offset 0
                              MemberDecorate 14(Buffer) 0 Offset 0
                              Decorate 14(Buffer) BufferBlock
                              Decorate 16 DescriptorSet 0
                              Decorate 16 Binding 0
                              MemberDecorate 26(gl_PerVertex) 0 BuiltIn Position
                              MemberDecorate 26(gl_PerVertex) 1 BuiltIn PointSize
                              MemberDecorate 26(gl_PerVertex) 2 BuiltIn ClipDistance
                              MemberDecorate 26(gl_PerVertex) 3 BuiltIn CullDistance
                              Decorate 26(gl_PerVertex) Block
               2:             TypeVoid
               3:             TypeFunction 2
               6:             TypeBool
               7:             TypeVector 6(bool) 2
         8(Data):             TypeStruct 7(bvec2)
               9:             TypePointer Function 8(Data)
              11:             TypeInt 32 0
              12:             TypeVector 11(int) 2
        13(Data):             TypeStruct 12(ivec2)
      14(Buffer):             TypeStruct 13(Data)
              15:             TypePointer Uniform 14(Buffer)
              16:     15(ptr) Variable Uniform
              17:             TypeInt 32 1
              18:     17(int) Constant 0
              19:             TypePointer Uniform 13(Data)
              22:             TypeFloat 32
              23:             TypeVector 22(float) 4
              24:     11(int) Constant 1
              25:             TypeArray 22(float) 24
26(gl_PerVertex):             TypeStruct 23(fvec4) 22(float) 25 25
              27:             TypePointer Output 26(gl_PerVertex)
              28:     27(ptr) Variable Output
              29:             TypePointer Function 23(fvec4)
              31:             TypePointer Function 7(bvec2)
              37:   22(float) Constant 0
              38:   23(fvec4) ConstantComposite 37 37 37 37
              40:   22(float) Constant 1065353216
              41:   23(fvec4) ConstantComposite 40 40 40 40
              43:             TypePointer Output 23(fvec4)
         4(main):           2 Function None 3
               5:             Label
           10(d):      9(ptr) Variable Function
              30:     29(ptr) Variable Function
              20:     19(ptr) AccessChain 16 18
              21:    13(Data) Load 20
                              Store 10(d) 21
              32:     31(ptr) AccessChain 10(d) 18
              33:    7(bvec2) Load 32
              34:     6(bool) CompositeExtract 33 0
                              SelectionMerge 36 None
                              BranchConditional 34 35 39
              35:               Label
                                Store 30 38
                                Branch 36
              39:               Label
                                Store 30 41
                                Branch 36
              36:             Label
              42:   23(fvec4) Load 30
              44:     43(ptr) AccessChain 28 18
                              Store 44 42
                              Return
                              FunctionEnd

The instruction Store 10(d) 21 violates the spec (the destination and source have different types) and the bool->uint32 information is lost in this case (no conversion instructions are emitted) when the structure is loaded from a block. So I think the original commit causes more problems.

[johnkslang]

The instruction "Store 18 16" violates the SPIR-V spec, which requires the destination and source operand should have the same type.

This basic issue was covered by issue #162, which discusses that this is partially implemented.

Also, I think that just making them integers is somewhat bad policy

It is the client APIs (at least Vulkan and OpenGL) that say how a bool is represented as a 32-bit integer in a UBO/SSBO. It is SPIR-V that says a bool is abstract, and provides enough instructions to convert.

But I think a bitcast instruction should be inserted

The design should not include bitcasts. It is OpSelect and OpNotEqual, etc, that convert between a bool and an int. Maybe I missed a subtler point?

Any time you have a bvecN in a UBO/SSBO, the code that's supposed to insert the int -> bool conversion simply doesn't.

Agreed the full implementation of int <-> bool conversions needs to be completed, and then it would be good to look again and see if there are issues. (I'm out of the office for a few more days before I can fix it myself.)

[amdrexu]

The design should not include bitcasts. It is OpSelect and OpNotEqual, etc, that convert between a bool and an int. Maybe I missed a subtler point?

Yes, I do it in my posted change exactly as you expect. I will think about how to handle bool in struct. Thanks for your confirmation.

[johnkslang]

This has been fixed for a while. Too many instances of this issue. ;)