On Tue, Apr 12, 2016 at 3:22 AM, Roland Scheidegger <[email protected]> wrote: > 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).
INSTANCE_ID_ZERO_BASE sounds indeed better. Marek _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
