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

[Alejandro Piñeiro]

On 13/08/18 16:14, Jason Ekstrand wrote:
> 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.
>
> On Mon, Aug 13, 2018 at 8:08 AM Alejandro Piñeiro
> <apinheiro at igalia.com <mailto:apinheiro at igalia.com>> wrote:
>
>     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
>     <mailto: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.
>
>
> I'd rewrite this comment more-or-less as follows:
>
> 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.
>
> With that,
>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>

Thanks for the explanation and quick review. Based on your explanation
at the top, and the updated comment, I also tweaked a little the git
commit message.

Just pushed the series.

Again, thanks.

>  
>
>     >         */
>     > -      *location = SYSTEM_VALUE_VERTEX_ID_ZERO_BASE;
>     > +      *location = SYSTEM_VALUE_VERTEX_ID;
>     >        set_mode_system_value(b, mode);
>     >        break;
>     >     case SpvBuiltInInstanceIndex:
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180813/82257b6a/attachment.html>