Am 12.04.2016 um 02:04 schrieb Jason Ekstrand: > > > On Mon, Apr 11, 2016 at 4:31 PM, Roland Scheidegger <[email protected] > <mailto:[email protected]>> 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 _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
