<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On OpenGL, a array of a simple type adds just one varying. So<br>
gl_transform_feedback_varying_info struct defined at mtypes.h includes<br>
the parameters Type (base_type) and Size (number of elements).<br>
<br>
This commit checks this when the recursive add_var_xfb_outputs call<br>
handles arrays, to ensure that just one is addded.<br>
<br>
RFC: Until this point, all changes were reasonable, but this change is<br>
(imho) ugly. My idea was introducing as less as possible changes on<br>
the code, specially on its logic/flow. But this commit is almost a<br>
hack. The ideal solution would be to change the focus of the recursive<br>
function, focusing on varyings, and at each varying, recursively add<br>
outputs. But that seems like an overkill for a pass that was<br>
originally intended for consumers only caring about the outputs. So<br>
perhaps ARB_gl_spirv should keep their own gathering pass, with<br>
vayings and outputs, and let this one untouched for those that only<br>
care on outputs.<br>
---<br>
 src/compiler/nir/nir_gather_xfb_info.c | 52 ++++++++++++++++++++++++++++------<br>
 1 file changed, 43 insertions(+), 9 deletions(-)<br>
<br>
diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c<br>
index 948b802a815..cb0e2724cab 100644<br>
--- a/src/compiler/nir/nir_gather_xfb_info.c<br>
+++ b/src/compiler/nir/nir_gather_xfb_info.c<br>
@@ -36,23 +36,59 @@ nir_gather_xfb_info_create(void *mem_ctx, uint16_t output_count, uint16_t varyin<br>
  Â  return xfb;<br>
 }<br>
<br>
+static bool<br>
+glsl_type_is_leaf(const struct glsl_type *type)<br>
+{<br>
+  Â if (glsl_type_is_struct(type) ||<br>
+  Â  Â  Â (glsl_type_is_array(type) &&<br>
+  Â  Â  Â  (glsl_type_is_array(glsl_get_array_element(type)) ||<br>
+  Â  Â  Â  Â glsl_type_is_struct(glsl_get_array_element(type))))) {<br></blockquote><div><br></div><div>I'm trying to understand exactly what this means.  From what you wrote here it looks like the following are all one varying:</div><div><br></div><div>float var[3];</div><div>vec2 var[3];</div><div>mat4 var[3];</div><div><br></div><div>but the following are not</div><div><br></div><div>struct S {</div><div>   float f;</div><div>   vec4 v;<br></div><div>};</div><div><br></div><div>S var[3];</div><div>float var[3][5];</div><div><br></div><div>Is this correct?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+  Â  Â  return false;<br>
+  Â } else {<br>
+  Â  Â  return true;<br>
+  Â }<br>
+}<br>
+<br>
+static void<br>
+add_var_xfb_varying(nir_xfb_info *xfb,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  nir_variable *var,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  unsigned offset,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  const struct glsl_type *type)<br>
+{<br>
+  Â nir_xfb_varying_info *varying = &xfb->varyings[xfb->varying_count++];<br>
+<br>
+  Â varying->type = type;<br>
+  Â varying->buffer = var->data.xfb_buffer;<br>
+  Â varying->offset = offset;<br>
+  Â xfb->buffers[var->data.xfb_buffer].varying_count++;<br>
+}<br>
+<br>
 static void<br>
 add_var_xfb_outputs(nir_xfb_info *xfb,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â nir_variable *var,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â unsigned *location,<br>
  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â unsigned *offset,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  const struct glsl_type *type)<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  const struct glsl_type *type,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  bool varying_added)<br>
 {<br>
  Â  if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) {<br>
  Â  Â  Â unsigned length = glsl_get_length(type);<br>
+  Â  Â  bool local_varying_added = varying_added;<br>
+<br>
  Â  Â  Â const struct glsl_type *child_type = glsl_get_array_element(type);<br>
+  Â  Â  if (glsl_type_is_leaf(child_type)) {<br>
+<br>
+  Â  Â  Â  Â add_var_xfb_varying(xfb, var, *offset, type);<br>
+  Â  Â  Â  Â local_varying_added = true;<br>
+  Â  Â  }<br>
+<br>
  Â  Â  Â for (unsigned i = 0; i < length; i++)<br>
-  Â  Â  Â  Â add_var_xfb_outputs(xfb, var, location, offset, child_type);<br>
+  Â  Â  Â  Â add_var_xfb_outputs(xfb, var, location, offset, child_type, local_varying_added);<br>
  Â  } else if (glsl_type_is_struct(type)) {<br>
  Â  Â  Â unsigned length = glsl_get_length(type);<br>
  Â  Â  Â for (unsigned i = 0; i < length; i++) {<br>
  Â  Â  Â  Â  const struct glsl_type *child_type = glsl_get_struct_field(type, i);<br>
-  Â  Â  Â  Â add_var_xfb_outputs(xfb, var, location, offset, child_type);<br>
+  Â  Â  Â  Â add_var_xfb_outputs(xfb, var, location, offset, child_type, varying_added);<br>
  Â  Â  Â }<br>
  Â  } else {<br>
  Â  Â  Â assert(var->data.xfb_buffer < NIR_MAX_XFB_BUFFERS);<br>
@@ -83,11 +119,9 @@ add_var_xfb_outputs(nir_xfb_info *xfb,<br>
  Â  Â  Â uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac;<br>
  Â  Â  Â unsigned location_frac = var->data.location_frac;<br>
<br>
-  Â  Â  nir_xfb_varying_info *varying = &xfb->varyings[xfb->varying_count++];<br>
-  Â  Â  varying->type = type;<br>
-  Â  Â  varying->buffer = var->data.xfb_buffer;<br>
-  Â  Â  varying->offset = *offset;<br>
-  Â  Â  xfb->buffers[var->data.xfb_buffer].varying_count++;<br>
+  Â  Â  if (!varying_added) {<br>
+  Â  Â  Â  Â add_var_xfb_varying(xfb, var, *offset, type);<br>
+  Â  Â  }<br>
<br>
  Â  Â  Â assert(attrib_slots <= 2);<br>
  Â  Â  Â for (unsigned s = 0; s < attrib_slots; s++) {<br>
@@ -171,7 +205,7 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx)<br>
<br>
  Â  Â  Â  Â  unsigned location = var->data.location;<br>
  Â  Â  Â  Â  unsigned offset = var->data.offset;<br>
-  Â  Â  Â  Â add_var_xfb_outputs(xfb, var, &location, &offset, var->type);<br>
+  Â  Â  Â  Â add_var_xfb_outputs(xfb, var, &location, &offset, var->type, false);<br>
  Â  Â  Â }<br>
  Â  }<br>
  Â  assert(xfb->output_count == num_outputs);<br>
-- <br>
2.14.1<br>
<br>
</blockquote></div></div>