[Mesa-dev] [PATCH 02/10] spirv: Make VertexIndex and VertexId both non-zero-based

[Alejandro Piñeiro]

This is the only pending patch from this series, would appreciate a
review. As everything involving the VertexIndex/Id, it is somewhat
tricky. I'm CCing Kenneth too as he was involved on the OpenGL mess with
gl_VertexId and similar.

FWIW, I already executed the Intel CI, and there are no regressions.


On 09/08/18 15:43, Alejandro Piñeiro wrote:
> From: Neil Roberts <nroberts at igalia.com>
>
> GLSL has gl_VertexID which is supposed to be non-zero-based.
>
> SPIR-V has both VertexIndex and VertexId builtins whose meanings are
> defined by the APIs.
>
> Vulkan defines VertexIndex as being non-zero-based. I can’t find
> any mention of what VertexId is supposed to be.
>
> GL_ARB_spirv removes VertexIndex and defines VertexId to be the same
> as gl_VertexId (which is alo non-zero-based).
>
> Previously in Mesa it was treating VertexIndex as non-zero-based and
> VertexId as zero-based, so it was breaking for GL. This behaviour was
> apparently based on Khronos bug 14255. However that bug doesn’t seem
> to have made a final decision for VertexId.
>
> Assuming there really is no other definition for VertexId for Vulkan
> it seems better to just make them both have the same
> value.
> ---
>  src/compiler/spirv/vtn_variables.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c
> index 8dab86abd74..b92bda59828 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1011,15 +1011,16 @@ vtn_get_builtin_location(struct vtn_builder *b,
>     case SpvBuiltInCullDistance:
>        *location = VARYING_SLOT_CULL_DIST0;
>        break;
> -   case SpvBuiltInVertexIndex:
> -      *location = SYSTEM_VALUE_VERTEX_ID;
> -      set_mode_system_value(b, mode);
> -      break;
>     case SpvBuiltInVertexId:
> -      /* Vulkan defines VertexID to be zero-based and reserves the new
> -       * builtin keyword VertexIndex to indicate the non-zero-based value.
> +   case SpvBuiltInVertexIndex:
> +      /* For whatever reason, both of these are defined in the SPIR-V spec.
> +       * The Vulkan spec defines VertexIndex to be non-zero-based and doesn’t
> +       * mention VertexId. The ARB_gl_spirv spec defines VertexId to be the
> +       * same as gl_VertexID, which is non-zero-based, and removes
> +       * VertexIndex. Assuming there is no use for VertexId in Vulkan yet, we
> +       * can just make them both be the same.
>         */
> -      *location = SYSTEM_VALUE_VERTEX_ID_ZERO_BASE;
> +      *location = SYSTEM_VALUE_VERTEX_ID;
>        set_mode_system_value(b, mode);
>        break;
>     case SpvBuiltInInstanceIndex: