[Mesa-dev] [PATCH v4 4/6] spirv: Lower BaseVertex to FIRST_VERTEX instead of BASE_VERTEX

[Ian Romanick]

On 04/06/2018 11:00 PM, Jason Ekstrand wrote:
> On Fri, Apr 6, 2018 at 2:53 PM, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
> 
>     From: Neil Roberts <nroberts at igalia.com <mailto:nroberts at igalia.com>>
> 
>     The base vertex in Vulkan is different from GL in that for non-indexed
>     primitives the value is taken from the firstVertex parameter instead
>     of being set to zero. This coincides with the new
>     SYSTEM_VALUE_FIRST_VERTEX
>     instead of BASE_VERTEX.
> 
>     Reviewed-by: Ian Romanick <ian.d.romanick at intel.com
>     <mailto:ian.d.romanick at intel.com>>
>     ---
>      src/compiler/spirv/vtn_variables.c |  2 +-
>      src/intel/vulkan/genX_cmd_buffer.c | 16 ++++++++++++----
>      src/intel/vulkan/genX_pipeline.c   |  2 ++
>      3 files changed, 15 insertions(+), 5 deletions(-)
> 
>     diff --git a/src/compiler/spirv/vtn_variables.c
>     b/src/compiler/spirv/vtn_variables.c
>     index b2897407fb1..9bb7d5a575e 100644
>     --- a/src/compiler/spirv/vtn_variables.c
>     +++ b/src/compiler/spirv/vtn_variables.c
>     @@ -1296,7 +1296,7 @@ vtn_get_builtin_location(struct vtn_builder *b,
>            set_mode_system_value(b, mode);
>            break;
>         case SpvBuiltInBaseVertex:
>     -      *location = SYSTEM_VALUE_BASE_VERTEX;
>     +      *location = SYSTEM_VALUE_FIRST_VERTEX;
> 
> 
> Given that we are taking something called BaseVertex and using the
> FIRST_VERTEX location when there is a location called BASE_VERTEX, I
> think a comment is in order.

That is fair, but I struggled a bit to come up with something
non-circular.  The best I could get is:

            /* OpenGL gl_BaseVertex (SYSTEM_VALUE_BASE_VERTEX) is not the
             * same semantic as SPIR-V BaseVertex (SYSTEM_VALUE_FIRST_VERTEX).
             */

>            set_mode_system_value(b, mode);
>            break;
>         case SpvBuiltInBaseInstance:
>     diff --git a/src/intel/vulkan/genX_cmd_buffer.c
>     b/src/intel/vulkan/genX_cmd_buffer.c
>     index 3c703f6be44..7d190a4d5cf 100644
>     --- a/src/intel/vulkan/genX_cmd_buffer.c
>     +++ b/src/intel/vulkan/genX_cmd_buffer.c
>     @@ -2673,7 +2673,9 @@ void genX(CmdDraw)(
> 
>         genX(cmd_buffer_flush_state)(cmd_buffer);
> 
>     -   if (vs_prog_data->uses_basevertex ||
>     vs_prog_data->uses_baseinstance)
>     +   if (vs_prog_data->uses_firstvertex ||
>     +       vs_prog_data->uses_basevertex ||
>     +       vs_prog_data->uses_baseinstance)
> 
> 
> I was going to ask if this was needed.  Then I saw patch 5.
> 
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>
>  
> 
>            emit_base_vertex_instance(cmd_buffer, firstVertex,
>     firstInstance);
>         if (vs_prog_data->uses_drawid)
>            emit_draw_index(cmd_buffer, 0);
>     @@ -2711,7 +2713,9 @@ void genX(CmdDrawIndexed)(
> 
>         genX(cmd_buffer_flush_state)(cmd_buffer);
> 
>     -   if (vs_prog_data->uses_basevertex ||
>     vs_prog_data->uses_baseinstance)
>     +   if (vs_prog_data->uses_firstvertex ||
>     +       vs_prog_data->uses_basevertex ||
>     +       vs_prog_data->uses_baseinstance)
>            emit_base_vertex_instance(cmd_buffer, vertexOffset,
>     firstInstance);
>         if (vs_prog_data->uses_drawid)
>            emit_draw_index(cmd_buffer, 0);
>     @@ -2850,7 +2854,9 @@ void genX(CmdDrawIndirect)(
>            struct anv_bo *bo = buffer->bo;
>            uint32_t bo_offset = buffer->offset + offset;
> 
>     -      if (vs_prog_data->uses_basevertex ||
>     vs_prog_data->uses_baseinstance)
>     +      if (vs_prog_data->uses_firstvertex ||
>     +          vs_prog_data->uses_basevertex ||
>     +          vs_prog_data->uses_baseinstance)
>               emit_base_vertex_instance_bo(cmd_buffer, bo, bo_offset + 8);
>            if (vs_prog_data->uses_drawid)
>               emit_draw_index(cmd_buffer, i);
>     @@ -2889,7 +2895,9 @@ void genX(CmdDrawIndexedIndirect)(
>            uint32_t bo_offset = buffer->offset + offset;
> 
>            /* TODO: We need to stomp base vertex to 0 somehow */
>     -      if (vs_prog_data->uses_basevertex ||
>     vs_prog_data->uses_baseinstance)
>     +      if (vs_prog_data->uses_firstvertex ||
>     +          vs_prog_data->uses_basevertex ||
>     +          vs_prog_data->uses_baseinstance)
>               emit_base_vertex_instance_bo(cmd_buffer, bo, bo_offset + 12);
>            if (vs_prog_data->uses_drawid)
>               emit_draw_index(cmd_buffer, i);
>     diff --git a/src/intel/vulkan/genX_pipeline.c
>     b/src/intel/vulkan/genX_pipeline.c
>     index eb2d4147357..a473f42c7e1 100644
>     --- a/src/intel/vulkan/genX_pipeline.c
>     +++ b/src/intel/vulkan/genX_pipeline.c
>     @@ -98,6 +98,7 @@ emit_vertex_input(struct anv_pipeline *pipeline,
>         const bool needs_svgs_elem = vs_prog_data->uses_vertexid ||
>                                      vs_prog_data->uses_instanceid ||
>                                      vs_prog_data->uses_basevertex ||
>     +                                vs_prog_data->uses_firstvertex ||
>                                      vs_prog_data->uses_baseinstance;
> 
>         uint32_t elem_count = __builtin_popcount(elements) -
>     @@ -178,6 +179,7 @@ emit_vertex_input(struct anv_pipeline *pipeline,
>             * well.  Just do all or nothing.
>             */
>            uint32_t base_ctrl = (vs_prog_data->uses_basevertex ||
>     +                            vs_prog_data->uses_firstvertex ||
>                                  vs_prog_data->uses_baseinstance) ?
>                                 VFCOMP_STORE_SRC : VFCOMP_STORE_0;
> 
>     --
>     2.14.3
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>