[Mesa-dev] [PATCH] spirv/nir: adjust location assignment for the case of arrays of blocks
This is needed due how the types get rearranged after the struct splitting. So for example, this array of blocks: layout(location = 0) out block { vec4 v; vec3 v2; } x[2]; Would be splitted on two nir variables with the following types: * vec4 v[2] * vec3 v2[2] So we need to take into account the length of the array to avoid locations overlaps one with the other. --- Hi Jason, again, sending in advance patches, just in case you are working on the same. I was able to fix the location overlapping without all those crazy ideas about lowering array of blocks into individual blocks, by just adjusting the locations as this patch shows. FWIW, the resulting locations are equivalent to those that we get with GLSL IR, that results on a similar splitting. With this change I got the following working: * SPIR-V simple arrays of blocks input/outputs * The arrays of blocks inputs/outputs + interpolator qualifiers test I mentioned to you last week [1] when run its SPIR-V equivalent. * SPIR-V xfb tests using arrays of blocks, where the xfb offset are assigned to all block members. * SPIR-V xfb tests using arrays of blocks, where the xfb offset is assigned to just one member, so just that member is captured, although as many times as the array length (yes! afaiu by spec that needs to work) So now, the only pending thing is a cleanup and send the series to review. Specifically, I think that this series can be put on top of current master instead of the arb_gl_spirv. Will try that and send a final series this week or early next week. BR [1] https://github.com/Igalia/piglit/blob/master/tests/spec/glsl-1.50/execution/interface-block-interpolation-array.shader_test src/compiler/spirv/vtn_variables.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index a8f2fdfa534..87386cee42f 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1672,6 +1672,14 @@ add_missing_member_locations(struct vtn_variable *var, glsl_get_length(glsl_without_array(var->type->type)); int location = var->base_location; + /* To know if it is a interface block we can't ask directly for +* var->type->block because on the case of arrays of blocks, block is set +* on the array_element. +*/ + bool is_array_block = var->var->interface_type != NULL && + glsl_type_is_array(var->type->type); + int adjustment = is_array_block ? glsl_get_length(var->type->type) : 1; + for (unsigned i = 0; i < length; i++) { /* From the Vulkan spec: * @@ -1702,8 +1710,12 @@ add_missing_member_locations(struct vtn_variable *var, const struct glsl_type *member_type = glsl_get_struct_field(glsl_without_array(var->type->type), i); + /* For arrays of interface blocks we can't just add the attribute slots + * of a member type due how the splitting would rearrange the types, so + * we need to adjust for the array length in that case. + */ location += - glsl_count_attribute_slots(member_type, is_vertex_input); + glsl_count_attribute_slots(member_type, is_vertex_input) * adjustment; } } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] spirv/nir: adjust location assignment for the case of arrays of blocks
On 13/12/18 11:11 pm, Alejandro Piñeiro wrote: This is needed due how the types get rearranged after the struct splitting. So for example, this array of blocks: layout(location = 0) out block { vec4 v; vec3 v2; } x[2]; Would be splitted on two nir variables with the following types: * vec4 v[2] * vec3 v2[2] So we need to take into account the length of the array to avoid locations overlaps one with the other. --- Hi Jason, again, sending in advance patches, just in case you are working on the same. I was able to fix the location overlapping without all those crazy ideas about lowering array of blocks into individual blocks, by just adjusting the locations as this patch shows. FWIW, the resulting locations are equivalent to those that we get with GLSL IR, that results on a similar splitting. With this change I got the following working: * SPIR-V simple arrays of blocks input/outputs * The arrays of blocks inputs/outputs + interpolator qualifiers test I mentioned to you last week [1] when run its SPIR-V equivalent. * SPIR-V xfb tests using arrays of blocks, where the xfb offset are assigned to all block members. * SPIR-V xfb tests using arrays of blocks, where the xfb offset is assigned to just one member, so just that member is captured, although as many times as the array length (yes! afaiu by spec that needs to work) So now, the only pending thing is a cleanup and send the series to review. Specifically, I think that this series can be put on top of current master instead of the arb_gl_spirv. Will try that and send a final series this week or early next week. BR [1] https://github.com/Igalia/piglit/blob/master/tests/spec/glsl-1.50/execution/interface-block-interpolation-array.shader_test src/compiler/spirv/vtn_variables.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index a8f2fdfa534..87386cee42f 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1672,6 +1672,14 @@ add_missing_member_locations(struct vtn_variable *var, glsl_get_length(glsl_without_array(var->type->type)); int location = var->base_location; + /* To know if it is a interface block we can't ask directly for +* var->type->block because on the case of arrays of blocks, block is set +* on the array_element. +*/ + bool is_array_block = var->var->interface_type != NULL && + glsl_type_is_array(var->type->type); + int adjustment = is_array_block ? glsl_get_length(var->type->type) : 1; + I think you probably want to use glsl_get_aoa_size() here instead of glsl_get_length() ? for (unsigned i = 0; i < length; i++) { /* From the Vulkan spec: * @@ -1702,8 +1710,12 @@ add_missing_member_locations(struct vtn_variable *var, const struct glsl_type *member_type = glsl_get_struct_field(glsl_without_array(var->type->type), i); + /* For arrays of interface blocks we can't just add the attribute slots + * of a member type due how the splitting would rearrange the types, so + * we need to adjust for the array length in that case. + */ location += - glsl_count_attribute_slots(member_type, is_vertex_input); + glsl_count_attribute_slots(member_type, is_vertex_input) * adjustment; } } ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] spirv/nir: adjust location assignment for the case of arrays of blocks
On 14/12/18 0:54, Timothy Arceri wrote: > > > On 13/12/18 11:11 pm, Alejandro Piñeiro wrote: >> This is needed due how the types get rearranged after the struct >> splitting. >> >> So for example, this array of blocks: >> >> layout(location = 0) out block { >> vec4 v; >> vec3 v2; >> } x[2]; >> >> Would be splitted on two nir variables with the following types: >> * vec4 v[2] >> * vec3 v2[2] >> >> So we need to take into account the length of the array to avoid >> locations overlaps one with the other. >> --- >> >> >> Hi Jason, >> >> again, sending in advance patches, just in case you are working on the >> same. >> >> I was able to fix the location overlapping without all those crazy >> ideas about lowering array of blocks into individual blocks, by just >> adjusting the locations as this patch shows. >> >> FWIW, the resulting locations are equivalent to those that we get with >> GLSL IR, that results on a similar splitting. >> >> With this change I got the following working: >> * SPIR-V simple arrays of blocks input/outputs >> >> * The arrays of blocks inputs/outputs + interpolator qualifiers >> test I mentioned to you last week [1] when run its SPIR-V >> equivalent. >> >> * SPIR-V xfb tests using arrays of blocks, where the xfb offset are >> assigned to all block members. >> >> * SPIR-V xfb tests using arrays of blocks, where the xfb offset is >> assigned to just one member, so just that member is captured, >> although as many times as the array length (yes! afaiu by spec >> that needs to work) >> >> So now, the only pending thing is a cleanup and send the series to >> review. Specifically, I think that this series can be put on top of >> current master instead of the arb_gl_spirv. Will try that and send a >> final series this week or early next week. >> >> BR >> >> >> [1] >> https://github.com/Igalia/piglit/blob/master/tests/spec/glsl-1.50/execution/interface-block-interpolation-array.shader_test >> >> >> >> src/compiler/spirv/vtn_variables.c | 14 +- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/src/compiler/spirv/vtn_variables.c >> b/src/compiler/spirv/vtn_variables.c >> index a8f2fdfa534..87386cee42f 100644 >> --- a/src/compiler/spirv/vtn_variables.c >> +++ b/src/compiler/spirv/vtn_variables.c >> @@ -1672,6 +1672,14 @@ add_missing_member_locations(struct >> vtn_variable *var, >> glsl_get_length(glsl_without_array(var->type->type)); >> int location = var->base_location; >> + /* To know if it is a interface block we can't ask directly for >> + * var->type->block because on the case of arrays of blocks, >> block is set >> + * on the array_element. >> + */ >> + bool is_array_block = var->var->interface_type != NULL && >> + glsl_type_is_array(var->type->type); >> + int adjustment = is_array_block ? >> glsl_get_length(var->type->type) : 1; >> + > > I think you probably want to use glsl_get_aoa_size() here instead of > glsl_get_length() ? Ok, thanks for the suggestion, I will try it, and add some extra tests to verify it. Having said so, after cleaning this RFC series, I got some regressions with tessellation cts vulkan tests with Intel CI, starting from this commit. So unfourtunately, this is still WIP, and probably the final patch will be different. Again, thanks for the hint. > >> for (unsigned i = 0; i < length; i++) { >> /* From the Vulkan spec: >> * >> @@ -1702,8 +1710,12 @@ add_missing_member_locations(struct >> vtn_variable *var, >> const struct glsl_type *member_type = >> glsl_get_struct_field(glsl_without_array(var->type->type), >> i); >> + /* For arrays of interface blocks we can't just add the >> attribute slots >> + * of a member type due how the splitting would rearrange the >> types, so >> + * we need to adjust for the array length in that case. >> + */ >> location += >> - glsl_count_attribute_slots(member_type, is_vertex_input); >> + glsl_count_attribute_slots(member_type, is_vertex_input) * >> adjustment; >> } >> } >> > pEpkey.asc Description: application/pgp-keys ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev