<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 11, 2016 at 4:31 PM, Roland Scheidegger <<a href="mailto:sroland@vmware.com" target="_blank">sroland@vmware.com</a>> wrote: <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: > 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: </div></div>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...) </blockquote><div> </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. </div><div>--Jason </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, Reviewed-by: Roland Scheidegger <<a href="mailto:sroland@vmware.com">sroland@vmware.com</a>> <div class="HOEnZb"><div class="h5"> > + default: > + assert(!"Unexpected SYSTEM_VALUE_ enum"); > + return TGSI_SEMANTIC_COUNT; > + } > +} > > - /* Compute shaders > - */ > - TGSI_SEMANTIC_THREAD_ID, > - TGSI_SEMANTIC_BLOCK_ID, > - TGSI_SEMANTIC_GRID_SIZE, > -}; > > /** > * Make note of a branch to a label in the TGSI code. > @@ -6000,35 +6029,6 @@ st_translate_program( > assert(numInputs <= ARRAY_SIZE(t->inputs)); > assert(numOutputs <= ARRAY_SIZE(t->outputs)); > > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_FRONT_FACE] == > - TGSI_SEMANTIC_FACE); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_VERTEX_ID] == > - TGSI_SEMANTIC_VERTEXID); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_INSTANCE_ID] == > - TGSI_SEMANTIC_INSTANCEID); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_SAMPLE_ID] == > - TGSI_SEMANTIC_SAMPLEID); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_SAMPLE_POS] == > - TGSI_SEMANTIC_SAMPLEPOS); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_SAMPLE_MASK_IN] == > - TGSI_SEMANTIC_SAMPLEMASK); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_INVOCATION_ID] == > - TGSI_SEMANTIC_INVOCATIONID); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_VERTEX_ID_ZERO_BASE] == > - TGSI_SEMANTIC_VERTEXID_NOBASE); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_BASE_VERTEX] == > - TGSI_SEMANTIC_BASEVERTEX); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_TESS_COORD] == > - TGSI_SEMANTIC_TESSCOORD); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_HELPER_INVOCATION] == > - TGSI_SEMANTIC_HELPER_INVOCATION); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_LOCAL_INVOCATION_ID] == > - TGSI_SEMANTIC_THREAD_ID); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_WORK_GROUP_ID] == > - TGSI_SEMANTIC_BLOCK_ID); > - assert(_mesa_sysval_to_semantic[SYSTEM_VALUE_NUM_WORK_GROUPS] == > - TGSI_SEMANTIC_GRID_SIZE); > - > t = CALLOC_STRUCT(st_translate); > if (!t) { > ret = PIPE_ERROR_OUT_OF_MEMORY; > @@ -6215,7 +6215,7 @@ st_translate_program( > > for (i = 0; sysInputs; i++) { > if (sysInputs & (1 << i)) { > - unsigned semName = _mesa_sysval_to_semantic[i]; > + unsigned semName = _mesa_sysval_to_semantic(i); > > t->systemValues[i] = ureg_DECL_system_value(ureg, semName, 0); > > diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.h b/src/mesa/state_tracker/st_glsl_to_tgsi.h > index 729295b..774588a 100644 > --- a/src/mesa/state_tracker/st_glsl_to_tgsi.h > +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.h > @@ -63,7 +63,8 @@ st_translate_stream_output_info(struct glsl_to_tgsi_visitor *glsl_to_tgsi, > const GLuint outputMapping[], > struct pipe_stream_output_info *so); > > -extern const unsigned _mesa_sysval_to_semantic[SYSTEM_VALUE_MAX]; > +unsigned > +_mesa_sysval_to_semantic(unsigned sysval); > > #ifdef __cplusplus > } > diff --git a/src/mesa/state_tracker/st_mesa_to_tgsi.c b/src/mesa/state_tracker/st_mesa_to_tgsi.c > index 7a686b1..e1c79a5 100644 > --- a/src/mesa/state_tracker/st_mesa_to_tgsi.c > +++ b/src/mesa/state_tracker/st_mesa_to_tgsi.c > @@ -1074,7 +1074,7 @@ st_translate_mesa_program( > > for (i = 0; sysInputs; i++) { > if (sysInputs & (1 << i)) { > - unsigned semName = _mesa_sysval_to_semantic[i]; > + unsigned semName = _mesa_sysval_to_semantic(i); > > t->systemValues[i] = ureg_DECL_system_value(ureg, semName, 0); > > </div></div><div class="HOEnZb"><div class="h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>