<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Jason, just one thing here. Although I appreciate your
interest to understand how varyings are enumerated, I think that
we are diverting here, as in the end that would be something that
I would need to solve. I just wanted to know for the way to go.<br>
</p>
<p>The main question here is if we are really interested on adding
such complexity on the general xfb gathering pass. This RFC was
basically a way to show how much changes we would need, even for a
incomplete solution Im not totally happy.</p>
<p>So at this point, do you think that it is worth to add varying
computation to the general pass in the name of code reuse, or
should ARB_gl_spirv stick to their own gathering pass?<br>
</p>
<br>
<div class="moz-cite-prefix">On 10/11/18 12:13, Alejandro Piñeiro
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:aabf2b9a-801c-bb46-84ea-fc7014d3e02c@igalia.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
On 09/11/18 16:58, Jason Ekstrand wrote:<br>
<blockquote type="cite"
cite="mid:166f9327ec0.27ad.c6988b7ea6112e3e892765a0d4287e0c@jlekstrand.net">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<div dir="auto">
<div dir="auto"><span style="font-family: sans-serif;
font-size: 10pt;">On November 9, 2018 06:39:25 Alejandro
Piñeiro <a class="moz-txt-link-rfc2396E"
href="mailto:apinheiro@igalia.com"
moz-do-not-send="true"><apinheiro@igalia.com></a>
wrote:</span></div>
<div id="aqm-original" style="color: black;">
<div text="#000000" bgcolor="#ffffff"
class="aqm-original-body">
<div style="color: black;">
<blockquote type="cite" class="gmail_quote"
style="margin: 0 0 0 0.75ex; border-left: 1px solid
#808080; padding-left: 0.75ex;"> On 08/11/18 23:14,
Jason Ekstrand wrote:<br>
<blockquote type="cite"
cite="mid:CAOFGe944TmGLLemDQuP_=QpMj03MHGx2UNsqGUHCfMc6c2ed1g@mail.gmail.com">
<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"
moz-do-not-send="true">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>
</div>
</blockquote>
<br>
Yes, GLSL returns one varying per each one (Size 3).</blockquote>
</div>
</div>
</div>
<div dir="auto"><br>
</div>
<div dir="auto">Just to be clear, a matrix it array of
matrices is one varying?</div>
</div>
</blockquote>
<br>
Yep, and being more clear, for this shader:<br>
#version 150<br>
#extension GL_ARB_enhanced_layouts: require<br>
<br>
layout(xfb_offset = 0) out mat4 var[3];<br>
<br>
void main() {<br>
mat4 m4;<br>
<br>
gl_Position = vec4(0.0);<br>
<br>
var[0] = m4;<br>
}<br>
<br>
We get the following when we dump
gl_program::LinkedTransformFeedback, that is a struct
gl_transform_feedback_info defined at mtypes.h:<br>
<br>
[gl_transform_feedback_info]<br>
NumOuputs = 12, (OutputRegister, OutputBuffer, NumComponents,
StreamId, DstOffset, ComponentOffset) <br>
0:(31, 0, 4, 0, 0, 0)<br>
1:(32, 0, 4, 0, 4, 0)<br>
2:(33, 0, 4, 0, 8, 0)<br>
3:(34, 0, 4, 0, 12, 0)<br>
4:(35, 0, 4, 0, 16, 0)<br>
5:(36, 0, 4, 0, 20, 0)<br>
6:(37, 0, 4, 0, 24, 0)<br>
7:(38, 0, 4, 0, 28, 0)<br>
8:(39, 0, 4, 0, 32, 0)<br>
9:(40, 0, 4, 0, 36, 0)<br>
10:(41, 0, 4, 0, 40, 0)<br>
11:(42, 0, 4, 0, 44, 0)<br>
NumVarying=1, (Offset, Type, BufferIndex, Size, Name) <br>
0:( 0, GL_FLOAT_MAT4, 0, 3, var)<br>
ActiveBuffers=1, (Binding, NumVaryings, Stride, Stream):<br>
0:( 0, 1, 192, 0)<br>
<br>
FWIW, in some cases we are also getting a slightly different
amount of Outputs. But Im personally not really worried about that
as far as it keeps working. The number of varyings is somewhat
different as it is exposed through the program interface queries,
so (I assume) it should be consistent.<br>
<br>
<blockquote type="cite"
cite="mid:166f9327ec0.27ad.c6988b7ea6112e3e892765a0d4287e0c@jlekstrand.net">
<div dir="auto">
<div dir="auto"><br>
</div>
<div id="aqm-original" style="color: black;" dir="auto">
<div text="#000000" bgcolor="#ffffff"
class="aqm-original-body">
<div style="color: black;">
<blockquote type="cite" class="gmail_quote"
style="margin: 0 0 0 0.75ex; border-left: 1px solid
#808080; padding-left: 0.75ex;"><br>
<blockquote type="cite"
cite="mid:CAOFGe944TmGLLemDQuP_=QpMj03MHGx2UNsqGUHCfMc6c2ed1g@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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>
</div>
</blockquote>
<br>
<blockquote type="cite"
cite="mid:CAOFGe944TmGLLemDQuP_=QpMj03MHGx2UNsqGUHCfMc6c2ed1g@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div>float var[3][5];</div>
</div>
</div>
</blockquote>
<br>
I guess that you are asking for thos two cases because
this code is not handling it properly. You are right.
For the array of structs, our code is crashing. For
the array of arrays, it is enumerating four varyings.
One with three GL_FLOAT components, and three with
five GL_FLOAT components, instead of just three
varyings with five components. In my defense, I
already mentioned that it was wip code, but preferred
to agree on the way to go before keep working on it.<br>
<br>
For the GLSL case, the array of struct returns 6
varyings. And funny thing, for the array of arrays,
GLSL is handling the situation even worse. It returns
the following link error: "Failed to link: error:
Transform feedback varying var[0] undeclared." Just a
quick skim on the spec, I didn't see anything
preventing using aoa with transform feedback varyings,
so I guess that this is a bug due all the rules OpenGL
has in relation with variable names.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe944TmGLLemDQuP_=QpMj03MHGx2UNsqGUHCfMc6c2ed1g@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Is this correct?<br>
</div>
</div>
</div>
</blockquote>
<br>
Yes. FWIW, I will give you another two examples, from
the tests Im using as reference.<br>
<br>
## example 1 ##<br>
struct Array {<br>
float x2_out;<br>
};<br>
struct AoA {<br>
Array x2_Array[2];<br>
};<br>
struct S {<br>
float x1_out;<br>
AoA x2_AoA[2];<br>
float x3_out;<br>
};<br>
layout(xfb_offset = 0) out S s1;<br>
layout(xfb_offset = 0, xfb_buffer = 2) out struct
S2 {<br>
float y1_out;<br>
vec4 y2_out;<br>
} s2;<br>
<br>
GLSL returns the following varyings (on ARB_gl_spirv
we target to get the same, although without the names)<br>
NumVarying=8, (Offset, Type, BufferIndex, Size,
Name) <br>
0:( 0, GL_FLOAT, 0, 1, s1.x1_out)<br>
1:( 4, GL_FLOAT, 0, 1,
s1.x2_AoA[0].x2_Array[0].x2_out)<br>
2:( 8, GL_FLOAT, 0, 1,
s1.x2_AoA[0].x2_Array[1].x2_out)<br>
3:(12, GL_FLOAT, 0, 1,
s1.x2_AoA[1].x2_Array[0].x2_out)<br>
4:(16, GL_FLOAT, 0, 1,
s1.x2_AoA[1].x2_Array[1].x2_out)<br>
5:(20, GL_FLOAT, 0, 1, s1.x3_out)<br>
6:( 0, GL_FLOAT, 1, 1, s2.y1_out)<br>
7:( 4, GL_FLOAT_VEC4, 1, 1, s2.y2_out)<br>
<br>
## example 2 ##<br>
layout(xfb_offset = 0) out float x1_out;<br>
layout(xfb_offset = 4) out float x2_out[2];<br>
layout(xfb_offset = 12) out vec3 x3_out;<br>
layout(xfb_buffer = 2) out;<br>
layout(xfb_offset = 0, xfb_buffer = 2) out float
y1_out;<br>
layout(xfb_offset = 4) out vec4 y2_out<br>
<br>
GLSL returns the following varyings (on ARB_gl_spirv
we target to get the same, although without the names)<br>
NumVarying=5, (Offset, Type, BufferIndex, Size,
Name) <br>
0:( 0, GL_FLOAT, 0, 1, x1_out)<br>
1:( 4, GL_FLOAT, 0, 2, x2_out)<br>
2:(12, GL_FLOAT_VEC3, 0, 1, x3_out)<br>
3:( 0, GL_FLOAT, 1, 1, y1_out)<br>
4:( 4, GL_FLOAT_VEC4, 1, 1, y2_out)<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe944TmGLLemDQuP_=QpMj03MHGx2UNsqGUHCfMc6c2ed1g@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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>
</blockquote>
<br>
</blockquote>
</div>
</div>
<!-- body end --> </div>
<div dir="auto"><br>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>