Re: [Piglit] [PATCH] vulkan: Add some tests for block member decorations

apinheiro Thu, 24 Jan 2019 07:47:02 -0800


On 24/1/19 16:28, Lionel Landwerlin wrote:

Thanks a lot for explaining.


I was wondering whether it would be worth checking something like this :

(layout location = 1) out block myBlock {
    vec4 a;
    vec4 b;
}

And verify whether a would bet location 1 and b location 2.

So do you mean adding a new test, or modify the current one to base on 0instead of base on 1?


But this current version looks good to me :

Reviewed-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>



Thanks!

Side question that I cannot find the answer to in the spec, thefollowing would be illegal right? :
layout (location= 1) out block myOtherBlock {
   layout (location = 1) vec4 a;
   layout (location = 0) vec4 b;
   layout (location = 3) vec4 c;
   layout (location = 2) vec4 d;
};

Well, I don't think so. This is the spec quote that we included on thefirst patch of the mesa merge request:


       “If the structure type is a Block but without a Location, then each
        of its members must have a Location decoration. If it is a Block
        with a Location decoration, then its members are assigned
        consecutive locations in declaration order, starting from the
        first member which is initially the Block. Any member with its own
        Location decoration is assigned that location. Each remaining
        member is assigned the location after the immediately preceding
        member in declaration order.”

What I understand from here, is that the location used for the blockwould be the one used as "base_location" when assigning locations formembers that lacks it, specially at the beginning of the block. But ifthe member has a location, then we use it, and we also use it for thefollowing members. So for example, in this case:


layout (location = 5) out myOtherBlock {
   vec4 a;
   layout (location = 0) vec4 b;
   layout (location = 3) vec4 c;
   layout (location = 2) vec4 d;
};

'a' gets assigned 5, and the rest would get assigned 0, 3 and 2respectively. And I have just tested with glslang, and it is doing justthat.


Thanks again.

-Lionel

On 24/01/2019 13:17, apinheiro wrote:


On 24/1/19 12:28, Lionel Landwerlin wrote:

I'm not sure whether my understanding ofblock-layout-location.vk_shader_test is correct.Is the expectation that the location of %name (0) is added to thelocation of its field (a, b, c, d)?

Not sure if added is the proper word, but derived. So for thosemembers, SPIR-V doesn't include the location. The block has thelocation, so the members get a location based on it. So that specifictest comes from a block like this:


(layout location= 0) out block myBlock {

 vec4 a;

 vec4 b:

 vec4 c;

 vec4 d;

};

That glslang translates to SPIR-V with setting location 0 tomyBlock, but not setting a location for the members. Such memberswould get locations 0, 1, 2, 3 respectively. Those locations areassigned right now by the driver, at least on the anv case that Itested. Note that as mentioned on the original email, this is thecase that is working right now. I only tested it with anv, but thoselocation assignment is done on the spirv to nir pass, so that wouldaffect other Vulkan drivers using it.

FWIW, what we are trying to fix with the MR I sent to review [1], andtested with the tests on this patch, is basically the case wherethere is a explicit location for any block member, that doesn'tfollow the "default location assignment rule" based on the block baselocation. So for example, this:


layout (location= 0) out block myOtherBlock {

   layout (location = 1) vec4 a;

   layout (location = 0) vec4 b;

   layout (location = 3) vec4 c;

   layout (location = 2) vec4 d;

};


[1] https://gitlab.freedesktop.org/mesa/mesa/merge_requests/142


Thanks,

-Lionel

On 23/01/2019 15:07, Alejandro Piñeiro wrote:

From: Neil Roberts <nrobe...@igalia.com>

v2: imported to piglit from a example vkrunner examples branch, also
     updated description on the top comment (Alejandro Piñeiro)
---

This tests are intended to test the patches at the following mesa MR:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/142

Although FWIW, block-layout-location.vk_shader_test is passing right
now with just master. The other two tests require the first patch
included on that MR.

.../block-layout-location.vk_shader_test | 121+++++++++++++++++

  ...lock-member-layout-location.vk_shader_test |  69 ++++++++++

...block-mixed-layout-location.vk_shader_test | 126++++++++++++++++++

  3 files changed, 316 insertions(+)

create mode 100644tests/vulkan/shaders/block-layout-location.vk_shader_test create mode 100644tests/vulkan/shaders/block-member-layout-location.vk_shader_test create mode 100644tests/vulkan/shaders/block-mixed-layout-location.vk_shader_test

diff --gita/tests/vulkan/shaders/block-layout-location.vk_shader_testb/tests/vulkan/shaders/block-layout-location.vk_shader_test

new file mode 100644
index 000000000..3eb2c0402
--- /dev/null
+++ b/tests/vulkan/shaders/block-layout-location.vk_shader_test
@@ -0,0 +1,121 @@
+# Test that interface block members are correctly matched by explicit
+# location, when only the main variable has a location, so the
+# location of the members should be derived from this.
+#

+# Note that we include the spirv assembly. This is becausealthough we

+# used a GLSL shader as reference, we tweaked the SPIR-V generated
+
+[vertex shader spirv]
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450

+ OpEntryPoint Vertex %main "main" %name %_%piglit_vertex

+               OpSource GLSL 440
+               OpName %main "main"
+               OpName %block "block"
+               OpMemberName %block 0 "a"
+               OpMemberName %block 1 "b"
+               OpMemberName %block 2 "c"
+               OpMemberName %block 3 "d"
+               OpName %name "name"
+               OpName %gl_PerVertex "gl_PerVertex"
+               OpMemberName %gl_PerVertex 0 "gl_Position"
+               OpMemberName %gl_PerVertex 1 "gl_PointSize"
+               OpMemberName %gl_PerVertex 2 "gl_ClipDistance"
+               OpName %_ ""
+               OpName %piglit_vertex "piglit_vertex"
+               OpDecorate %block Block

+; Only the main name variable has a location. The locations of themembers

+; should be derived from this.
+               OpDecorate %name Location 0
+               OpMemberDecorate %gl_PerVertex 0 BuiltIn Position
+               OpMemberDecorate %gl_PerVertex 1 BuiltIn PointSize
+               OpMemberDecorate %gl_PerVertex 2 BuiltIn ClipDistance
+               OpDecorate %gl_PerVertex Block
+               OpDecorate %piglit_vertex Location 0
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+    %v4float = OpTypeVector %float 4
+      %block = OpTypeStruct %v4float %v4float %v4float %v4float
+%_ptr_Output_block = OpTypePointer Output %block
+       %name = OpVariable %_ptr_Output_block Output
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+    %float_1 = OpConstant %float 1
+    %float_0 = OpConstant %float 0

+ %15 = OpConstantComposite %v4float %float_1 %float_0%float_0 %float_1

+%_ptr_Output_v4float = OpTypePointer Output %v4float
+      %int_1 = OpConstant %int 1

+ %19 = OpConstantComposite %v4float %float_0 %float_1%float_0 %float_1

+      %int_2 = OpConstant %int 2

+ %22 = OpConstantComposite %v4float %float_0 %float_0%float_1 %float_1

+      %int_3 = OpConstant %int 3

+ %25 = OpConstantComposite %v4float %float_1 %float_1%float_1 %float_1

+       %uint = OpTypeInt 32 0
+     %uint_1 = OpConstant %uint 1
+%_arr_float_uint_1 = OpTypeArray %float %uint_1
+%gl_PerVertex = OpTypeStruct %v4float %float %_arr_float_uint_1
+%_ptr_Output_gl_PerVertex = OpTypePointer Output %gl_PerVertex
+          %_ = OpVariable %_ptr_Output_gl_PerVertex Output
+%_ptr_Input_v4float = OpTypePointer Input %v4float
+%piglit_vertex = OpVariable %_ptr_Input_v4float Input
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %17 = OpAccessChain %_ptr_Output_v4float %name %int_0
+               OpStore %17 %15
+         %20 = OpAccessChain %_ptr_Output_v4float %name %int_1
+               OpStore %20 %19
+         %23 = OpAccessChain %_ptr_Output_v4float %name %int_2
+               OpStore %23 %22
+         %26 = OpAccessChain %_ptr_Output_v4float %name %int_3
+               OpStore %26 %25
+         %35 = OpLoad %v4float %piglit_vertex
+         %36 = OpAccessChain %_ptr_Output_v4float %_ %int_0
+               OpStore %36 %35
+               OpReturn
+               OpFunctionEnd
+
+[fragment shader]
+#version 440
+
+layout(location = 0) in vec4 a;
+layout(location = 1) in vec4 b;
+layout(location = 2) in vec4 c;
+layout(location = 3) in vec4 d;
+
+layout(std140, push_constant) uniform block {
+        int i;
+};
+
+layout(location = 0) out vec4 color;
+
+void main()
+{
+        if (i == 0) {
+                color = a;
+        } else if (i == 1) {
+                color = b;
+        } else if (i == 2) {
+                color = c;
+        } else if (i == 3) {
+                color = d;
+        }
+}
+
+[test]
+uniform int 0 0
+draw rect -1 -1 1 1
+relative probe rect rgb (0.0, 0.0, 0.5, 0.5) (1.0, 0.0, 0.0)
+
+uniform int 0 1
+draw rect 0 -1 1 1
+relative probe rect rgb (0.5, 0.0, 0.5, 0.5) (0.0, 1.0, 0.0)
+
+uniform int 0 2
+draw rect -1 0 1 1
+relative probe rect rgb (0.0, 0.5, 0.5, 0.5) (0.0, 0.0, 1.0)
+
+uniform int 0 3
+draw rect 0 0 1 1
+relative probe rect rgb (0.5, 0.5, 0.5, 0.5) (1.0, 1.0, 1.0)

diff --gita/tests/vulkan/shaders/block-member-layout-location.vk_shader_testb/tests/vulkan/shaders/block-member-layout-location.vk_shader_test

new file mode 100644
index 000000000..dd23331b8
--- /dev/null
+++ b/tests/vulkan/shaders/block-member-layout-location.vk_shader_test
@@ -0,0 +1,69 @@
+# Test that interface block members are correctly matched by explicit
+# location, when we assign explicit location for all members of a
+# block, and unsorted.
+
+[vertex shader]
+#version 440
+
+layout(location = 0) in vec4 piglit_vertex;
+
+out block {
+        layout(location = 2) vec4 c;
+        layout(location = 3) vec4 d;
+        layout(location = 0) vec4 a;
+        layout(location = 1) vec4 b;
+} name;
+
+void main()
+{
+        name.a = vec4(1.0, 0.0, 0.0, 1.0);
+        name.b = vec4(0.0, 1.0, 0.0, 1.0);
+        name.c = vec4(0.0, 0.0, 1.0, 1.0);
+        name.d = vec4(1.0, 1.0, 1.0, 1.0);
+
+        gl_Position = piglit_vertex;
+}
+
+[fragment shader]
+#version 440
+
+layout(location = 0) in vec4 a;
+layout(location = 1) in vec4 b;
+layout(location = 2) in vec4 c;
+layout(location = 3) in vec4 d;
+
+layout(std140, push_constant) uniform block {
+        int i;
+};
+
+layout(location = 0) out vec4 color;
+
+void main()
+{
+        if (i == 0) {
+                color = a;
+        } else if (i == 1) {
+                color = b;
+        } else if (i == 2) {
+                color = c;
+        } else if (i == 3) {
+                color = d;
+        }
+}
+
+[test]
+uniform int 0 0
+draw rect -1 -1 1 1
+relative probe rect rgb (0.0, 0.0, 0.5, 0.5) (1.0, 0.0, 0.0)
+
+uniform int 0 1
+draw rect 0 -1 1 1
+relative probe rect rgb (0.5, 0.0, 0.5, 0.5) (0.0, 1.0, 0.0)
+
+uniform int 0 2
+draw rect -1 0 1 1
+relative probe rect rgb (0.0, 0.5, 0.5, 0.5) (0.0, 0.0, 1.0)
+
+uniform int 0 3
+draw rect 0 0 1 1
+relative probe rect rgb (0.5, 0.5, 0.5, 0.5) (1.0, 1.0, 1.0)

diff --gita/tests/vulkan/shaders/block-mixed-layout-location.vk_shader_testb/tests/vulkan/shaders/block-mixed-layout-location.vk_shader_test

new file mode 100644
index 000000000..750dd2b29
--- /dev/null
+++ b/tests/vulkan/shaders/block-mixed-layout-location.vk_shader_test
@@ -0,0 +1,126 @@
+# Test that interface block members are correctly matched by explicit
+# location. In this case the main variable has a location value, but
+# there is also one member with an explicit value.
+#