<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">In order to allow nir_gather_xfb_info to be used on OpenGL,<br>
specifically ARB_gl_spirv.<br>
<br>
So, from OpenGL 4.6 spec, section 11.1.2.1, "Output Variables":<br>
<br>
    "outputs specifying both an *XfbBuffer* and an *Offset* are<br>
     captured, while outputs not specifying both of these are not<br>
     captured. Values are captured each time the shader writes to such<br>
     a decorated object."<br>
<br>
This implies that are captured if both are present, and not if one of<br>
those are lacking. Technically, it doesn't explicitly point that<br>
having just one or the other is a mistake. In some cases, glslang is<br>
adding some extra XfbBuffer without XfbOffset around, and mentioning<br>
that technically that is not a bug (see issue#1526)<br>
<br>
And for the case of Vulkan, as the same glslang issue mentions, it is<br>
not clear if that should be a mistake or not. But even if it is a<br>
mistake, it is not really needed to be checked on the driver, and we<br>
can let the validation layers to check that.<br>
---<br>
 src/compiler/nir/nir_gather_xfb_info.c | 23 ++++++++++++++++++++---<br>
 1 file changed, 20 insertions(+), 3 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 e282bba0081..f5d831c6567 100644<br>
--- a/src/compiler/nir/nir_gather_xfb_info.c<br>
+++ b/src/compiler/nir/nir_gather_xfb_info.c<br>
@@ -109,9 +109,16 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx)<br>
    nir_foreach_variable(var, &shader->outputs) {<br>
       if (var->data.explicit_xfb_buffer ||<br>
           var->data.explicit_xfb_stride) {<br>
-         assert(var->data.explicit_xfb_buffer &&<br>
-                var->data.explicit_xfb_stride &&<br>
-                var->data.explicit_offset);<br>
+<br>
+         /* OpenGL points that both are needed to capture the output, but<br>
+          * doesn't direcly imply that it is a mistake having one but not the<br>
+          * other.<br>
+          */<br>
+         if (!var->data.explicit_xfb_buffer ||<br>
+             !var->data.explicit_offset) {<br></blockquote><div><br></div><div>Why not just change the check above to "var->data.explicit_xfb_buffer && var->data.explicit_offset" and not bother with two checks?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            continue;<br>
+         }<br>
+<br>
          num_outputs += glsl_count_attribute_slots(var->type, false);<br>
       }<br>
    }<br>
@@ -124,6 +131,16 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx)<br>
    nir_foreach_variable(var, &shader->outputs) {<br>
       if (var->data.explicit_xfb_buffer ||<br>
           var->data.explicit_xfb_stride) {<br>
+<br>
+         /* OpenGL points that both are needed to capture the output, but<br>
+          * doesn't direcly imply that it is a mistake having one but not the<br>
+          * other.<br>
+          */<br>
+         if (!var->data.explicit_xfb_buffer ||<br>
+             !var->data.explicit_offset) {<br></blockquote><div><br></div><div>Same here.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            continue;<br>
+         }<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>
-- <br>
2.14.1<br>
<br>
</blockquote></div></div>