On 19/9/18 7:59 pm, Alejandro Piñeiro wrote:
On 19/09/18 07:20, Timothy Arceri wrote:
On 16/9/18 2:18 am, Alejandro Piñeiro wrote:
For this interfaces, the inner members are added only once as uniforms
or resources, in opposite to other cases, like a uniform array of
structs.
---
src/compiler/glsl/gl_nir_link_uniforms.c | 26
++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c
b/src/compiler/glsl/gl_nir_link_uniforms.c
index 00995fb3f76..c692cd0171f 100644
--- a/src/compiler/glsl/gl_nir_link_uniforms.c
+++ b/src/compiler/glsl/gl_nir_link_uniforms.c
@@ -498,11 +498,33 @@ gl_nir_link_uniforms(struct gl_context *ctx,
state.current_var = var;
+ /*
+ * From OpenGL 4.6 spec, section 7.3.1.1, "Naming Active
Resources":
+ *
+ * "* For an active shader storage block member declared
as an
+ * array of an aggregate type, an entry will be
generated only
+ * for the first array element, regardless of its
type. Such
+ * block members are referred to as top-level arrays.
If the
+ * block member is an aggregate type, the enumeration
rules are
+ * then applied recursively.
+ * * For an active interface block not declared as an
array of
+ * block instances, a single entry will be generated,
using the
+ * block name from the shader source."
+ *
+ * So for the UBO and SSBO case, we expand only the array
element
+ * type.
+ */
+ const struct glsl_type *type = var->type;
+ if (nir_variable_is_in_block(var) &&
+ glsl_type_is_array(type)) {
+ type = glsl_get_array_element(type);
This also strips away vector and matrix type information while leaving
arrays for anything but a single dimension array.
Well, but the idea behind this patch is getting a single dimension
array.
I think you mean getting a single element of an array?
We are using the type to populate the uniforms, and as mentioned
on the spec quote, for ubo/ssbo and recursively for his entries, it is
done just once, no matter the dimension of the ubo/ssbo array.
Also not sure why do you mean that we would lose matrix and vector type
information. Could you give an example.
The code for glsl_get_array_element() is:
const glsl_type *
glsl_get_array_element(const glsl_type* type)
{
if (type->is_matrix())
return type->column_type();
else if (type->is_vector())
return type->get_scalar_type();
return type->fields.array;
}
I was thinking you might end up stripping away the vec and matrix type
but as you have glsl_type_is_array(type) in the if condition it is fine.
FWIW, this patch is needed to get this test working:
https://github.com/Igalia/piglit/blob/arb_gl_spirv-series5-ubo-ssbo-v1/tests/spec/arb_gl_spirv/execution/ubo/array-complex.shader_test
and just in case, I have just writing a similar test, slightly more
complex, but with array members and array of matrices members:
https://github.com/Igalia/piglit/blob/arb_gl_spirv-series5-ubo-ssbo-v1/tests/spec/arb_gl_spirv/execution/ubo/array-complex-2.shader_test
And the uniform and variable count is the same on both GLSL and SPIR-V.
Are you sure you don't want the following instead?
type = glsl_get_array_element(type);
Sorry, but isn't that what we are already doing? Or do you mean that we
should made the call always, without the is_in_block and is_array checks?
Apologizes I pasted the wrong thing I meant:
type = glsl_without_array(type);
However looking more closely at the code and the spec I see that's not
what you want.
However I'm starting to think the quote from the spec and this code have
nothing to do with each other.
The spec is taking about top level block member arrays. For example:
layout (std140, binding = 5, row_major) uniform ComponentsBlock
{
float c1[3];
} components;
However neither of the examples you gave have top level arrays:
layout (std140, binding = 5, row_major) uniform ComponentsBlock
{
float c1;
vec2 c2;
S s;
} components[2];
My guess is you are using this to remove the array from the block itself
which is applied when the block is split into separate vars. However the
logic in this patch will fail when you have arrays of arrays for example:
layout (std140, binding = 5, row_major) uniform ComponentsBlock
{
float c1;
vec2 c2;
S s;
} components[2][2];
Or even something like:
layout (std140, binding = 5, row_major) uniform ComponentsBlock
{
float c1[3];
vec2 c2[3];
S s[3];
} components[2];
It's late here and I've only skimmed over other patches in the series so
if I'm missing something please correct me.
+ }
+
struct type_tree_entry *type_tree =
- build_type_tree_for_type(var->type);
+ build_type_tree_for_type(type);
state.current_type = type_tree;
- int res = nir_link_uniform(ctx, prog, sh->Program,
shader_type, var->type,
+ int res = nir_link_uniform(ctx, prog, sh->Program,
shader_type, type,
location, &state);
free_type_tree(type_tree);
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
