[Mesa-dev] [PATCH] st/mesa: replace _mesa_sysval_to_semantic table with function

[Roland Scheidegger]

Am 12.04.2016 um 02:04 schrieb Jason Ekstrand:
> 
> 
> On Mon, Apr 11, 2016 at 4:31 PM, Roland Scheidegger <sroland at vmware.com
> <mailto:sroland at vmware.com>> wrote:
> 
>     Am 12.04.2016 um 00:39 schrieb Brian Paul:
>     > Instead of using an array indexed by SYSTEM_VALUE_x, just use a
>     > switch statement.  This fixes a regression caused by inserting new
>     > SYSTEM_VALUE_ enums but not updating the mapping to TGSI semantics.
>     >
>     > v2: fix a few switch statement mistakes for compute-related enums
>     > ---
>     >  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 132
>     ++++++++++++++---------------
>     >  src/mesa/state_tracker/st_glsl_to_tgsi.h   |   3 +-
>     >  src/mesa/state_tracker/st_mesa_to_tgsi.c   |   2 +-
>     >  3 files changed, 69 insertions(+), 68 deletions(-)
>     >
>     > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>     b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>     > index b9ab7ae..5f037da 100644
>     > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>     > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>     > @@ -5192,43 +5192,72 @@ struct st_translate {
>     >  };
>     >
>     >  /** Map Mesa's SYSTEM_VALUE_x to TGSI_SEMANTIC_x */
>     > -const unsigned _mesa_sysval_to_semantic[SYSTEM_VALUE_MAX] = {
>     > -   /* Vertex shader
>     > -    */
>     > -   TGSI_SEMANTIC_VERTEXID,
>     > -   TGSI_SEMANTIC_INSTANCEID,
>     > -   TGSI_SEMANTIC_VERTEXID_NOBASE,
>     > -   TGSI_SEMANTIC_BASEVERTEX,
>     > -   TGSI_SEMANTIC_BASEINSTANCE,
>     > -   TGSI_SEMANTIC_DRAWID,
>     > -
>     > -   /* Geometry shader
>     > -    */
>     > -   TGSI_SEMANTIC_INVOCATIONID,
>     > -
>     > -   /* Fragment shader
>     > -    */
>     > -   TGSI_SEMANTIC_POSITION,
>     > -   TGSI_SEMANTIC_FACE,
>     > -   TGSI_SEMANTIC_SAMPLEID,
>     > -   TGSI_SEMANTIC_SAMPLEPOS,
>     > -   TGSI_SEMANTIC_SAMPLEMASK,
>     > -   TGSI_SEMANTIC_HELPER_INVOCATION,
>     > -
>     > -   /* Tessellation shaders
>     > -    */
>     > -   TGSI_SEMANTIC_TESSCOORD,
>     > -   TGSI_SEMANTIC_VERTICESIN,
>     > -   TGSI_SEMANTIC_PRIMID,
>     > -   TGSI_SEMANTIC_TESSOUTER,
>     > -   TGSI_SEMANTIC_TESSINNER,
>     > +unsigned
>     > +_mesa_sysval_to_semantic(unsigned sysval)
>     > +{
>     > +   switch (sysval) {
>     > +   /* Vertex shader */
>     > +   case SYSTEM_VALUE_VERTEX_ID:
>     > +      return TGSI_SEMANTIC_VERTEXID;
>     > +   case SYSTEM_VALUE_INSTANCE_ID:
>     > +      return TGSI_SEMANTIC_INSTANCEID;
>     > +   case SYSTEM_VALUE_VERTEX_ID_ZERO_BASE:
>     > +      return TGSI_SEMANTIC_VERTEXID_NOBASE;
>     > +   case SYSTEM_VALUE_BASE_VERTEX:
>     > +      return TGSI_SEMANTIC_BASEVERTEX;
>     > +   case SYSTEM_VALUE_BASE_INSTANCE:
>     > +      return TGSI_SEMANTIC_BASEINSTANCE;
>     > +   case SYSTEM_VALUE_DRAW_ID:
>     > +      return TGSI_SEMANTIC_DRAWID;
>     > +
>     > +   /* Geometry shader */
>     > +   case SYSTEM_VALUE_INVOCATION_ID:
>     > +      return TGSI_SEMANTIC_INVOCATIONID;
>     > +
>     > +   /* Fragment shader */
>     > +   case SYSTEM_VALUE_FRAG_COORD:
>     > +      return TGSI_SEMANTIC_POSITION;
>     > +   case SYSTEM_VALUE_FRONT_FACE:
>     > +      return TGSI_SEMANTIC_FACE;
>     > +   case SYSTEM_VALUE_SAMPLE_ID:
>     > +      return TGSI_SEMANTIC_SAMPLEID;
>     > +   case SYSTEM_VALUE_SAMPLE_POS:
>     > +      return TGSI_SEMANTIC_SAMPLEPOS;
>     > +   case SYSTEM_VALUE_SAMPLE_MASK_IN:
>     > +      return TGSI_SEMANTIC_SAMPLEMASK;
>     > +   case SYSTEM_VALUE_HELPER_INVOCATION:
>     > +      return TGSI_SEMANTIC_HELPER_INVOCATION;
>     > +
>     > +   /* Tessellation shader */
>     > +   case SYSTEM_VALUE_TESS_COORD:
>     > +      return TGSI_SEMANTIC_TESSCOORD;
>     > +   case SYSTEM_VALUE_VERTICES_IN:
>     > +      return TGSI_SEMANTIC_VERTICESIN;
>     > +   case SYSTEM_VALUE_PRIMITIVE_ID:
>     > +      return TGSI_SEMANTIC_PRIMID;
>     > +   case SYSTEM_VALUE_TESS_LEVEL_OUTER:
>     > +      return TGSI_SEMANTIC_TESSOUTER;
>     > +   case SYSTEM_VALUE_TESS_LEVEL_INNER:
>     > +      return TGSI_SEMANTIC_TESSINNER;
>     > +
>     > +   /* Compute shader */
>     > +   case SYSTEM_VALUE_LOCAL_INVOCATION_ID:
>     > +      return TGSI_SEMANTIC_THREAD_ID;
>     > +   case SYSTEM_VALUE_WORK_GROUP_ID:
>     > +      return TGSI_SEMANTIC_BLOCK_ID;
>     > +   case SYSTEM_VALUE_NUM_WORK_GROUPS:
>     > +      return TGSI_SEMANTIC_GRID_SIZE;
>     > +
>     > +   /* Unhandled */
>     > +   case SYSTEM_VALUE_LOCAL_INVOCATION_INDEX:
>     > +   case SYSTEM_VALUE_GLOBAL_INVOCATION_ID:
>     > +   case SYSTEM_VALUE_VERTEX_CNT:
>     If you wanted to include all existing values, you've missed the new
>     SYSTEM_VALUE_INSTANCE_INDEX here.
> 
>     (fwiw I find the names confusing - we have:
>     SYSTEM_VALUE_VERTEX_ID - includes baseindex
>     SYSTEM_VALUE_VERTEX_ID_ZERO_BASE - without baseindex
>     SYSTEM_VALUE_INSTANCE_ID - without baseinstance
>     SYSTEM_VALUE_INSTANCE_INDEX - includes baseinstance
>     And yes I'm aware the inconsistent naming is due to GL's naming
>     essentially, but still...)
> 
> 
> 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.
> --Jason

Oh, I wasn't even aware Vulkan provided both. I see though actually they
are all there in SPIR-V, refering to Vulkan API spec, which only has the
_INDEX ones (but I'm not quite uptodate there, seems to miss half the
builtins mentioned in spir-v so that's probably expected). Albeit
missing the gl _BASE ones (but of course if you have both _ID and _INDEX
you don't need them).

I agree renaming them to match vulkan would be more consistent, but just
as confusing due to gl. Can't say I'm a big fan of vulkan's naming there
neither though, consistent or not, if I see ID vs. INDEX I'd have no
idea which one now includes the base... Maybe it would help if
INSTANCE_ID be renamed to INSTANCE_ID_ZERO_BASE instead or something
(albeit that doesn't match what's currently done in tgsi neither -
because up to now no api provided instance id including the base).

But anyway, I guess not really a big deal...

Roland