<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">I hate decorations... 1 and 2 are </div><div class="gmail_quote">Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div class="gmail_quote"> On Mon, Jan 9, 2017 at 7:16 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Sun, 2017-01-08 at 21:26 -0800, Kenneth Graunke wrote: > We need to: > - handle the extra array level for per-vertex varyings > - handle the patch qualifier correctly > - assign varying locations > > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > --- > src/compiler/spirv/vtn_private.h | 1 + > src/compiler/spirv/vtn_variables.c | 56 > ++++++++++++++++++++++++++++++++++---- > 2 files changed, 52 insertions(+), 5 deletions(-) > > diff --git a/src/compiler/spirv/vtn_private.h > b/src/compiler/spirv/vtn_private.h > index 9302611803f..8b450106543 100644 > --- a/src/compiler/spirv/vtn_private.h > +++ b/src/compiler/spirv/vtn_private.h > @@ -280,6 +280,7 @@ struct vtn_variable { > unsigned descriptor_set; > unsigned binding; > unsigned input_attachment_index; > + bool patch; > > nir_variable *var; > nir_variable **members; > diff --git a/src/compiler/spirv/vtn_variables.c > b/src/compiler/spirv/vtn_variables.c > index e3845365bdd..dcd0609626b 100644 > --- a/src/compiler/spirv/vtn_variables.c > +++ b/src/compiler/spirv/vtn_variables.c > @@ -935,10 +935,19 @@ vtn_get_builtin_location(struct vtn_builder *b, > unreachable("invalid stage for SpvBuiltInViewportIndex"); > break; > case SpvBuiltInTessLevelOuter: > + *location = VARYING_SLOT_TESS_LEVEL_OUTER; > + break; > case SpvBuiltInTessLevelInner: > + *location = VARYING_SLOT_TESS_LEVEL_INNER; > + break; > case SpvBuiltInTessCoord: > + *location = SYSTEM_VALUE_TESS_COORD; > + set_mode_system_value(mode); > + break; > case SpvBuiltInPatchVertices: > - unreachable("no tessellation support"); > + *location = SYSTEM_VALUE_VERTICES_IN; > + set_mode_system_value(mode); > + break; > case SpvBuiltInFragCoord: > *location = VARYING_SLOT_POS; > assert(*mode == nir_var_shader_in); > @@ -1052,6 +1061,11 @@ apply_var_decoration(struct vtn_builder *b, > nir_variable *nir_var, > vtn_get_builtin_location(b, builtin, &nir_var->data.location, > &mode); > nir_var->data.mode = mode; > > + if (builtin == SpvBuiltInTessLevelOuter || > + builtin == SpvBuiltInTessLevelInner) { > + nir_var->data.compact = true; > + } > + </div></div>This is a nitpick so feel free to ignore it: maybe we we want to put the two ifs below in a single 'else if' block after this hunk. <div class="HOEnZb"><div class="h5"></div></div></blockquote><div> </div><div>I think I actually like it better the way he wrote it since they're setting unrelated things. :P </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > if (builtin == SpvBuiltInFragCoord || builtin == > SpvBuiltInSamplePosition) > nir_var->data.origin_upper_left = b->origin_upper_left; > > @@ -1076,7 +1090,7 @@ apply_var_decoration(struct vtn_builder *b, > nir_variable *nir_var, > break; /* Do nothing with these here */ > > case SpvDecorationPatch: > - vtn_warn("Tessellation not yet supported"); > + nir_var->data.patch = true; > break; > > case SpvDecorationLocation: > @@ -1116,6 +1130,15 @@ apply_var_decoration(struct vtn_builder *b, > nir_variable *nir_var, > } > > static void > +var_is_patch_cb(struct vtn_builder *b, struct vtn_value *val, int > member, > + const struct vtn_decoration *dec, void > *out_is_patch) > +{ > + if (dec->decoration == SpvDecorationPatch) { > + *((bool *) out_is_patch) = true; > + } > +} > + > +static void > var_decoration_cb(struct vtn_builder *b, struct vtn_value *val, int > member, > const struct vtn_decoration *dec, void *void_var) > { > @@ -1132,6 +1155,9 @@ var_decoration_cb(struct vtn_builder *b, struct > vtn_value *val, int member, > case SpvDecorationInputAttachmentIndex: > vtn_var->input_attachment_index = dec->literals[0]; > return; > + case SpvDecorationPatch: > + vtn_var->patch = true; > + break; > default: > break; > } > @@ -1162,7 +1188,7 @@ var_decoration_cb(struct vtn_builder *b, struct > vtn_value *val, int member, > } else if (vtn_var->mode == vtn_variable_mode_input || > vtn_var->mode == vtn_variable_mode_output) { > is_vertex_input = false; > - location += VARYING_SLOT_VAR0; > + location += vtn_var->patch ? VARYING_SLOT_PATCH0 : > VARYING_SLOT_VAR0; > } else { > unreachable("Location must be on input or output > variable"); > } > @@ -1209,6 +1235,24 @@ var_decoration_cb(struct vtn_builder *b, > struct vtn_value *val, int member, > } > } > > +static bool > +is_per_vertex_inout(const struct vtn_variable *var, gl_shader_stage > stage) > +{ > + if (var->patch || !glsl_type_is_array(var->type->type)) > + return false; > + > + if (var->mode == vtn_variable_mode_input) { > + return stage == MESA_SHADER_TESS_CTRL || > + stage == MESA_SHADER_TESS_EVAL || > + stage == MESA_SHADER_GEOMETRY; > + } > + > + if (var->mode == vtn_variable_mode_output) > + return stage == MESA_SHADER_TESS_CTRL; > + > + return false; > +} > + > void > vtn_handle_variables(struct vtn_builder *b, SpvOp opcode, > const uint32_t *w, unsigned count) > @@ -1308,6 +1352,9 @@ vtn_handle_variables(struct vtn_builder *b, > SpvOp opcode, > > case vtn_variable_mode_input: > case vtn_variable_mode_output: { > + var->patch = false; > + vtn_foreach_decoration(b, val, var_is_patch_cb, &var- > >patch); > + > /* For inputs and outputs, we immediately split > structures. This > * is for a couple of reasons. For one, builtins may all > come in > * a struct and we really want those split out into > separate > @@ -1318,8 +1365,7 @@ vtn_handle_variables(struct vtn_builder *b, > SpvOp opcode, > > int array_length = -1; > struct vtn_type *interface_type = var->type; > - if (b->shader->stage == MESA_SHADER_GEOMETRY && > - glsl_type_is_array(var->type->type)) { > + if (is_per_vertex_inout(var, b->shader->stage)) { > /* In Geometry shaders (and some tessellation), inputs > come > * in per-vertex arrays. However, some builtins come in > * non-per-vertex, hence the need for the is_array > check. In </div></div><div class="HOEnZb"><div class="h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>