<div dir="ltr">I think this is ok.  The difference in implementation is a bit of an artifact of history.  When we were working on Vulkan 1.0, we wanted to make things consistent.  One suggestion for how to do that was to have Vertex/InstanceId which was zero-based and Vertex/InstanceIndex which included the base offset.  Then people decided that having two wasn't worth the bother so we threw out the ID variants and just kept the index variants.  In Vulkan VertexId and InstanceId have no meaning and are pretty much just reserved for GL at this point.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 13, 2018 at 8:08 AM Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is the only pending patch from this series, would appreciate a<br>
review. As everything involving the VertexIndex/Id, it is somewhat<br>
tricky. I'm CCing Kenneth too as he was involved on the OpenGL mess with<br>
gl_VertexId and similar.<br>
<br>
FWIW, I already executed the Intel CI, and there are no regressions.<br>
<br>
<br>
On 09/08/18 15:43, Alejandro Piñeiro wrote:<br>
> From: Neil Roberts <<a href="mailto:nroberts@igalia.com" target="_blank">nroberts@igalia.com</a>><br>
><br>
> GLSL has gl_VertexID which is supposed to be non-zero-based.<br>
><br>
> SPIR-V has both VertexIndex and VertexId builtins whose meanings are<br>
> defined by the APIs.<br>
><br>
> Vulkan defines VertexIndex as being non-zero-based. I can’t find<br>
> any mention of what VertexId is supposed to be.<br>
><br>
> GL_ARB_spirv removes VertexIndex and defines VertexId to be the same<br>
> as gl_VertexId (which is alo non-zero-based).<br>
><br>
> Previously in Mesa it was treating VertexIndex as non-zero-based and<br>
> VertexId as zero-based, so it was breaking for GL. This behaviour was<br>
> apparently based on Khronos bug 14255. However that bug doesn’t seem<br>
> to have made a final decision for VertexId.<br>
><br>
> Assuming there really is no other definition for VertexId for Vulkan<br>
> it seems better to just make them both have the same<br>
> value.<br>
> ---<br>
>  src/compiler/spirv/vtn_variables.c | 15 ++++++++-------<br>
>  1 file changed, 8 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c<br>
> index 8dab86abd74..b92bda59828 100644<br>
> --- a/src/compiler/spirv/vtn_variables.c<br>
> +++ b/src/compiler/spirv/vtn_variables.c<br>
> @@ -1011,15 +1011,16 @@ vtn_get_builtin_location(struct vtn_builder *b,<br>
>     case SpvBuiltInCullDistance:<br>
>        *location = VARYING_SLOT_CULL_DIST0;<br>
>        break;<br>
> -   case SpvBuiltInVertexIndex:<br>
> -      *location = SYSTEM_VALUE_VERTEX_ID;<br>
> -      set_mode_system_value(b, mode);<br>
> -      break;<br>
>     case SpvBuiltInVertexId:<br>
> -      /* Vulkan defines VertexID to be zero-based and reserves the new<br>
> -       * builtin keyword VertexIndex to indicate the non-zero-based value.<br>
> +   case SpvBuiltInVertexIndex:<br>
> +      /* For whatever reason, both of these are defined in the SPIR-V spec.<br>
> +       * The Vulkan spec defines VertexIndex to be non-zero-based and doesn’t<br>
> +       * mention VertexId. The ARB_gl_spirv spec defines VertexId to be the<br>
> +       * same as gl_VertexID, which is non-zero-based, and removes<br>
> +       * VertexIndex. Assuming there is no use for VertexId in Vulkan yet, we<br>
> +       * can just make them both be the same.<br></blockquote><div><br></div><div>I'd rewrite this comment more-or-less as follows:</div><div><br></div><div>The Vulkan spec defines VertexIndex to be non-zero-based and doesn't allowe VertexId.  The ARB_gl_spirv spec defines VertexId to be the same as gl_VertexID, which is non-zer-based, and removes VertexIndex.  Since they're both defined to be non-zero-based, we use SYSTEM_VALUE_VERTEX_ID for both.</div><div><br></div><div>With that,</div><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>         */<br>
> -      *location = SYSTEM_VALUE_VERTEX_ID_ZERO_BASE;<br>
> +      *location = SYSTEM_VALUE_VERTEX_ID;<br>
>        set_mode_system_value(b, mode);<br>
>        break;<br>
>     case SpvBuiltInInstanceIndex:<br>
<br>
</blockquote></div></div>