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

[Jason Ekstrand]

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>
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>
> >
> > 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>


> >         */
> > -      *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/53bf7e1a/attachment.html>