[Mesa-dev] [PATCH 02/11] spirv: Add tessellation varying and built-in support.

[Iago Toral]

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 <kenneth at whitecape.org>
> ---
>  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;
> +      }
> +

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.

>        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