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 > <apinhe...@igalia.com <mailto:apinhe...@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 <nrobe...@igalia.com > <mailto:nrobe...@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 <ja...@jlekstrand.net > <mailto:ja...@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: >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
