<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 13/08/18 16:14, Jason Ekstrand wrote:<br>
<blockquote type="cite"
cite="mid:CAOFGe96-pm_vy_tYaEYB==V62UtNY_T-Uft2i=z=bdjF0ch+oQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<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"
moz-do-not-send="true">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"
moz-do-not-send="true">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" moz-do-not-send="true">jason@jlekstrand.net</a>><br>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
Just pushed the series.<br>
<br>
Again, thanks.<br>
<br>
<blockquote type="cite"
cite="mid:CAOFGe96-pm_vy_tYaEYB==V62UtNY_T-Uft2i=z=bdjF0ch+oQ@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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>
</blockquote>
<br>
</body>
</html>