[Mesa-dev] [PATCH 02/10] spirv: Make VertexIndex and VertexId both non-zero-based
apinheiro at igalia.com
Mon Aug 13 14:30:42 UTC 2018
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
> > 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
Just pushed the series.
> > */
> > - *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...
More information about the mesa-dev