[Mesa-dev] [PATCH] st/mesa: replace _mesa_sysval_to_semantic table with function
Marek Olšák
maraeo at gmail.com
Tue Apr 12 10:03:36 UTC 2016
On Tue, Apr 12, 2016 at 3:22 AM, Roland Scheidegger <sroland at vmware.com> wrote:
> 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).
INSTANCE_ID_ZERO_BASE sounds indeed better.
Marek
More information about the mesa-dev
mailing list