<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 11, 2016 at 4:31 PM, Roland Scheidegger <span dir="ltr"><<a href="mailto:sroland@vmware.com" target="_blank">sroland@vmware.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Am 12.04.2016 um 00:39 schrieb Brian Paul:<br>
> Instead of using an array indexed by SYSTEM_VALUE_x, just use a<br>
> switch statement.  This fixes a regression caused by inserting new<br>
> SYSTEM_VALUE_ enums but not updating the mapping to TGSI semantics.<br>
><br>
> v2: fix a few switch statement mistakes for compute-related enums<br>
> ---<br>
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 132 ++++++++++++++---------------<br>
>  src/mesa/state_tracker/st_glsl_to_tgsi.h   |   3 +-<br>
>  src/mesa/state_tracker/st_mesa_to_tgsi.c   |   2 +-<br>
>  3 files changed, 69 insertions(+), 68 deletions(-)<br>
><br>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp<br>
> index b9ab7ae..5f037da 100644<br>
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp<br>
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp<br>
> @@ -5192,43 +5192,72 @@ struct st_translate {<br>
>  };<br>
><br>
>  /** Map Mesa's SYSTEM_VALUE_x to TGSI_SEMANTIC_x */<br>
> -const unsigned _mesa_sysval_to_semantic[SYSTEM_VALUE_MAX] = {<br>
> -   /* Vertex shader<br>
> -    */<br>
> -   TGSI_SEMANTIC_VERTEXID,<br>
> -   TGSI_SEMANTIC_INSTANCEID,<br>
> -   TGSI_SEMANTIC_VERTEXID_NOBASE,<br>
> -   TGSI_SEMANTIC_BASEVERTEX,<br>
> -   TGSI_SEMANTIC_BASEINSTANCE,<br>
> -   TGSI_SEMANTIC_DRAWID,<br>
> -<br>
> -   /* Geometry shader<br>
> -    */<br>
> -   TGSI_SEMANTIC_INVOCATIONID,<br>
> -<br>
> -   /* Fragment shader<br>
> -    */<br>
> -   TGSI_SEMANTIC_POSITION,<br>
> -   TGSI_SEMANTIC_FACE,<br>
> -   TGSI_SEMANTIC_SAMPLEID,<br>
> -   TGSI_SEMANTIC_SAMPLEPOS,<br>
> -   TGSI_SEMANTIC_SAMPLEMASK,<br>
> -   TGSI_SEMANTIC_HELPER_INVOCATION,<br>
> -<br>
> -   /* Tessellation shaders<br>
> -    */<br>
> -   TGSI_SEMANTIC_TESSCOORD,<br>
> -   TGSI_SEMANTIC_VERTICESIN,<br>
> -   TGSI_SEMANTIC_PRIMID,<br>
> -   TGSI_SEMANTIC_TESSOUTER,<br>
> -   TGSI_SEMANTIC_TESSINNER,<br>
> +unsigned<br>
> +_mesa_sysval_to_semantic(unsigned sysval)<br>
> +{<br>
> +   switch (sysval) {<br>
> +   /* Vertex shader */<br>
> +   case SYSTEM_VALUE_VERTEX_ID:<br>
> +      return TGSI_SEMANTIC_VERTEXID;<br>
> +   case SYSTEM_VALUE_INSTANCE_ID:<br>
> +      return TGSI_SEMANTIC_INSTANCEID;<br>
> +   case SYSTEM_VALUE_VERTEX_ID_ZERO_BASE:<br>
> +      return TGSI_SEMANTIC_VERTEXID_NOBASE;<br>
> +   case SYSTEM_VALUE_BASE_VERTEX:<br>
> +      return TGSI_SEMANTIC_BASEVERTEX;<br>
> +   case SYSTEM_VALUE_BASE_INSTANCE:<br>
> +      return TGSI_SEMANTIC_BASEINSTANCE;<br>
> +   case SYSTEM_VALUE_DRAW_ID:<br>
> +      return TGSI_SEMANTIC_DRAWID;<br>
> +<br>
> +   /* Geometry shader */<br>
> +   case SYSTEM_VALUE_INVOCATION_ID:<br>
> +      return TGSI_SEMANTIC_INVOCATIONID;<br>
> +<br>
> +   /* Fragment shader */<br>
> +   case SYSTEM_VALUE_FRAG_COORD:<br>
> +      return TGSI_SEMANTIC_POSITION;<br>
> +   case SYSTEM_VALUE_FRONT_FACE:<br>
> +      return TGSI_SEMANTIC_FACE;<br>
> +   case SYSTEM_VALUE_SAMPLE_ID:<br>
> +      return TGSI_SEMANTIC_SAMPLEID;<br>
> +   case SYSTEM_VALUE_SAMPLE_POS:<br>
> +      return TGSI_SEMANTIC_SAMPLEPOS;<br>
> +   case SYSTEM_VALUE_SAMPLE_MASK_IN:<br>
> +      return TGSI_SEMANTIC_SAMPLEMASK;<br>
> +   case SYSTEM_VALUE_HELPER_INVOCATION:<br>
> +      return TGSI_SEMANTIC_HELPER_INVOCATION;<br>
> +<br>
> +   /* Tessellation shader */<br>
> +   case SYSTEM_VALUE_TESS_COORD:<br>
> +      return TGSI_SEMANTIC_TESSCOORD;<br>
> +   case SYSTEM_VALUE_VERTICES_IN:<br>
> +      return TGSI_SEMANTIC_VERTICESIN;<br>
> +   case SYSTEM_VALUE_PRIMITIVE_ID:<br>
> +      return TGSI_SEMANTIC_PRIMID;<br>
> +   case SYSTEM_VALUE_TESS_LEVEL_OUTER:<br>
> +      return TGSI_SEMANTIC_TESSOUTER;<br>
> +   case SYSTEM_VALUE_TESS_LEVEL_INNER:<br>
> +      return TGSI_SEMANTIC_TESSINNER;<br>
> +<br>
> +   /* Compute shader */<br>
> +   case SYSTEM_VALUE_LOCAL_INVOCATION_ID:<br>
> +      return TGSI_SEMANTIC_THREAD_ID;<br>
> +   case SYSTEM_VALUE_WORK_GROUP_ID:<br>
> +      return TGSI_SEMANTIC_BLOCK_ID;<br>
> +   case SYSTEM_VALUE_NUM_WORK_GROUPS:<br>
> +      return TGSI_SEMANTIC_GRID_SIZE;<br>
> +<br>
> +   /* Unhandled */<br>
> +   case SYSTEM_VALUE_LOCAL_INVOCATION_INDEX:<br>
> +   case SYSTEM_VALUE_GLOBAL_INVOCATION_ID:<br>
> +   case SYSTEM_VALUE_VERTEX_CNT:<br>
</div></div>If you wanted to include all existing values, you've missed the new<br>
SYSTEM_VALUE_INSTANCE_INDEX here.<br>
<br>
(fwiw I find the names confusing - we have:<br>
SYSTEM_VALUE_VERTEX_ID - includes baseindex<br>
SYSTEM_VALUE_VERTEX_ID_ZERO_BASE - without baseindex<br>
SYSTEM_VALUE_INSTANCE_ID - without baseinstance<br>
SYSTEM_VALUE_INSTANCE_INDEX - includes baseinstance<br>
And yes I'm aware the inconsistent naming is due to GL's naming<br>
essentially, but still...)<br></blockquote><div><br></div><div>Would you like a patch to rename VERTEX_ID to VERTEX_INDEX and VERTEX_ID_ZERO_BASE to VERTEX_ID.  That's sort-of the convention that was chosen during the Vulkan discussions to remove confusion.  I'd happily write such a patch.  Unfortunately, there would be confusion because gl_VertexID would be VERTEX_INDEX while gl_InstanceID would be INSTANCE_INDEX.  Fun fun.<br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Either way, thanks for fixing this,<br>
Reviewed-by: Roland Scheidegger <<a href="mailto:sroland@vmware.com">sroland@vmware.com</a>><br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<br>
> +   default:<br>
> +      assert(!"Unexpected SYSTEM_VALUE_ enum");<br>
> +      return TGSI_SEMANTIC_COUNT;<br>
> +   }<br>
> +}<br>
><br>
> -   /* Compute shaders<br>
> -    */<br>
> -   TGSI_SEMANTIC_THREAD_ID,<br>
> -   TGSI_SEMANTIC_BLOCK_ID,<br>
> -   TGSI_SEMANTIC_GRID_SIZE,<br>
> -};<br>
><br>
>  /**<br>
>   * Make note of a branch to a label in the TGSI code.<br>
> @@ -6000,35 +6029,6 @@ st_translate_program(<br>
>     assert(numInputs <= ARRAY_SIZE(t->inputs));<br>
>     assert(numOutputs <= ARRAY_SIZE(t->outputs));<br>
><br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_FRONT_FACE] ==<br>
> -          TGSI_SEMANTIC_FACE);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_VERTEX_ID] ==<br>
> -          TGSI_SEMANTIC_VERTEXID);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_INSTANCE_ID] ==<br>
> -          TGSI_SEMANTIC_INSTANCEID);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_SAMPLE_ID] ==<br>
> -          TGSI_SEMANTIC_SAMPLEID);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_SAMPLE_POS] ==<br>
> -          TGSI_SEMANTIC_SAMPLEPOS);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_SAMPLE_MASK_IN] ==<br>
> -          TGSI_SEMANTIC_SAMPLEMASK);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_INVOCATION_ID] ==<br>
> -          TGSI_SEMANTIC_INVOCATIONID);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_VERTEX_ID_ZERO_BASE] ==<br>
> -          TGSI_SEMANTIC_VERTEXID_NOBASE);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_BASE_VERTEX] ==<br>
> -          TGSI_SEMANTIC_BASEVERTEX);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_TESS_COORD] ==<br>
> -          TGSI_SEMANTIC_TESSCOORD);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_HELPER_INVOCATION] ==<br>
> -          TGSI_SEMANTIC_HELPER_INVOCATION);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_LOCAL_INVOCATION_ID] ==<br>
> -          TGSI_SEMANTIC_THREAD_ID);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_WORK_GROUP_ID] ==<br>
> -          TGSI_SEMANTIC_BLOCK_ID);<br>
> -   assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_NUM_WORK_GROUPS] ==<br>
> -          TGSI_SEMANTIC_GRID_SIZE);<br>
> -<br>
>     t = CALLOC_STRUCT(st_translate);<br>
>     if (!t) {<br>
>        ret = PIPE_ERROR_OUT_OF_MEMORY;<br>
> @@ -6215,7 +6215,7 @@ st_translate_program(<br>
><br>
>        for (i = 0; sysInputs; i++) {<br>
>           if (sysInputs & (1 << i)) {<br>
> -            unsigned semName = _mesa_sysval_to_semantic[i];<br>
> +            unsigned semName = _mesa_sysval_to_semantic(i);<br>
><br>
>              t->systemValues[i] = ureg_DECL_system_value(ureg, semName, 0);<br>
><br>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.h b/src/mesa/state_tracker/st_glsl_to_tgsi.h<br>
> index 729295b..774588a 100644<br>
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.h<br>
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.h<br>
> @@ -63,7 +63,8 @@ st_translate_stream_output_info(struct glsl_to_tgsi_visitor *glsl_to_tgsi,<br>
>                                  const GLuint outputMapping[],<br>
>                                  struct pipe_stream_output_info *so);<br>
><br>
> -extern const unsigned _mesa_sysval_to_semantic[SYSTEM_VALUE_MAX];<br>
> +unsigned<br>
> +_mesa_sysval_to_semantic(unsigned sysval);<br>
><br>
>  #ifdef __cplusplus<br>
>  }<br>
> diff --git a/src/mesa/state_tracker/st_mesa_to_tgsi.c b/src/mesa/state_tracker/st_mesa_to_tgsi.c<br>
> index 7a686b1..e1c79a5 100644<br>
> --- a/src/mesa/state_tracker/st_mesa_to_tgsi.c<br>
> +++ b/src/mesa/state_tracker/st_mesa_to_tgsi.c<br>
> @@ -1074,7 +1074,7 @@ st_translate_mesa_program(<br>
><br>
>        for (i = 0; sysInputs; i++) {<br>
>           if (sysInputs & (1 << i)) {<br>
> -            unsigned semName = _mesa_sysval_to_semantic[i];<br>
> +            unsigned semName = _mesa_sysval_to_semantic(i);<br>
><br>
>              t->systemValues[i] = ureg_DECL_system_value(ureg, semName, 0);<br>
><br>
><br>
<br>
</div></div><div class="HOEnZb"><div class="h5">_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>