Re: [Mesa-dev] [PATCH 1/9] compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID

[Antia Puentes]

Hi,

On 30/11/17 21:43, Neil Roberts wrote:


Kenneth Graunke  writes:


We have a number of similar names now:

SYSTEM_VALUE_BASE_VERTEX
SYSTEM_VALUE_BASE_VERTEX_ID
SYSTEM_VALUE_VERTEX_ID
SYSTEM_VALUE_VERTEX_ID_ZERO_BASE

BASE_VERTEX and BASE_VERTEX_ID are really similar names, and honestly
either one seems like it could be the name for gl_BaseVertex.  I'm
afraid it would be easy to mix them up by mistake.  IMHO, it would be
nice to pick a different word, just to keep some distinction between
the two fairly related concepts...

Perhaps SYSTEM_VALUE_FIRST_VERTEX...?  That's only half the meaning,
but it at least uses a different word, and makes you think "do I want
BASE_VERTEX or FIRST_VERTEX?"

Yes, naming this thing is really difficult. I’m not sure if you noticed,
but for Vulkan, the BaseVertex builtin should actually have the value of
BASE_VERTEX_ID unlike GL. So if we rename BASE_VERTEX to something
without “base vertex” in it then it will still be confusing for Vulkan.
So effectively the descriptive names are like:

SYSTEM_VALUE_BASE_VERTEX_ON_GL_BUT_NOT_VULKAN
SYSTEM_VALUE_BASE_VERTEX_ON_VULKAN_OR_OFFSET_FOR_VERTEX_ID_ON_GL

I’m not sure whether that’s enough of an argument against FIRST_VERTEX
though, so personally I don’t really mind either way.

Antia, what do you think?

I am fine renaming it to FIRST_VERTEX. I have sent a second version of 
the series with the renaming and addressing other feedback given by Kenneth.

Thanks.

Regards.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID

[Neil Roberts]

Kenneth Graunke  writes:

> We have a number of similar names now:
>
>SYSTEM_VALUE_BASE_VERTEX
>SYSTEM_VALUE_BASE_VERTEX_ID
>SYSTEM_VALUE_VERTEX_ID
>SYSTEM_VALUE_VERTEX_ID_ZERO_BASE
>
> BASE_VERTEX and BASE_VERTEX_ID are really similar names, and honestly
> either one seems like it could be the name for gl_BaseVertex.  I'm
> afraid it would be easy to mix them up by mistake.  IMHO, it would be
> nice to pick a different word, just to keep some distinction between
> the two fairly related concepts...
>
> Perhaps SYSTEM_VALUE_FIRST_VERTEX...?  That's only half the meaning,
> but it at least uses a different word, and makes you think "do I want
> BASE_VERTEX or FIRST_VERTEX?"

Yes, naming this thing is really difficult. I’m not sure if you noticed,
but for Vulkan, the BaseVertex builtin should actually have the value of
BASE_VERTEX_ID unlike GL. So if we rename BASE_VERTEX to something
without “base vertex” in it then it will still be confusing for Vulkan.
So effectively the descriptive names are like:

SYSTEM_VALUE_BASE_VERTEX_ON_GL_BUT_NOT_VULKAN
SYSTEM_VALUE_BASE_VERTEX_ON_VULKAN_OR_OFFSET_FOR_VERTEX_ID_ON_GL

I’m not sure whether that’s enough of an argument against FIRST_VERTEX
though, so personally I don’t really mind either way.

Antia, what do you think?

Regards,
- Neil


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/9] compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID

[Kenneth Graunke]

On Friday, November 10, 2017 9:53:29 AM PST Antia Puentes wrote:
> This VS system value will contain the value passed as 
> for indexed draw calls or the value passed as  for non-indexed
> draw calls. It will be used to calculate the gl_VertexID as
> SYSTEM_VALUE_VERTEX_ID_ZERO_BASE plus SYSTEM_VALUE_BASE_VERTEX_ID.
> Note that the current calculation which uses SYSTEM_VALUE_BASE_VERTEX
> is not right, as gl_BaseVertex should be zero for non-indexed calls.
> 
> Reviewed-by: Neil Roberts 
> ---
>  src/compiler/shader_enums.c |  1 +
>  src/compiler/shader_enums.h | 15 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/src/compiler/shader_enums.c b/src/compiler/shader_enums.c
> index b2ca80b49c2..cd8c256f227 100644
> --- a/src/compiler/shader_enums.c
> +++ b/src/compiler/shader_enums.c
> @@ -215,6 +215,7 @@ gl_system_value_name(gl_system_value sysval)
>   ENUM(SYSTEM_VALUE_INSTANCE_ID),
>   ENUM(SYSTEM_VALUE_INSTANCE_INDEX),
>   ENUM(SYSTEM_VALUE_VERTEX_ID_ZERO_BASE),
> + ENUM(SYSTEM_VALUE_BASE_VERTEX_ID),
>   ENUM(SYSTEM_VALUE_BASE_VERTEX),
>   ENUM(SYSTEM_VALUE_BASE_INSTANCE),
>   ENUM(SYSTEM_VALUE_DRAW_ID),
> diff --git a/src/compiler/shader_enums.h b/src/compiler/shader_enums.h
> index 9d229d4199e..7437cccdd0a 100644
> --- a/src/compiler/shader_enums.h
> +++ b/src/compiler/shader_enums.h
> @@ -474,6 +474,21 @@ typedef enum
>  */
> SYSTEM_VALUE_BASE_VERTEX,
>  
> +
> +   /**
> +* Depending on the type of the draw call (indexed or non-indexed),
> +* is the value of \c basevertex passed to \c glDrawElementsBaseVertex and
> +* similar, or is the value of \c first passed to \c glDrawArrays and
> +* similar.
> +*
> +* \note
> +* It can be used to calculate the \c SYSTEM_VALUE_VERTEX_ID as
> +* \c SYSTEM_VALUE_VERTEX_ID_ZERO_BASE plus \c 
> SYSTEM_VALUE_BASE_VERTEX_ID.
> +*
> +* \sa SYSTEM_VALUE_VERTEX_ID_ZERO_BASE, SYSTEM_VALUE_VERTEX_ID
> +*/
> +   SYSTEM_VALUE_BASE_VERTEX_ID,
> +

We have a number of similar names now:

   SYSTEM_VALUE_BASE_VERTEX
   SYSTEM_VALUE_BASE_VERTEX_ID
   SYSTEM_VALUE_VERTEX_ID
   SYSTEM_VALUE_VERTEX_ID_ZERO_BASE

BASE_VERTEX and BASE_VERTEX_ID are really similar names, and honestly
either one seems like it could be the name for gl_BaseVertex.  I'm
afraid it would be easy to mix them up by mistake.  IMHO, it would be
nice to pick a different word, just to keep some distinction between
the two fairly related concepts...

Perhaps SYSTEM_VALUE_FIRST_VERTEX...?  That's only half the meaning,
but it at least uses a different word, and makes you think "do I want
BASE_VERTEX or FIRST_VERTEX?"

> /**
>  * Value of \c baseinstance passed to instanced draw entry points
>  *
> 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/9] compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID

[Antia Puentes]

This VS system value will contain the value passed as 
for indexed draw calls or the value passed as  for non-indexed
draw calls. It will be used to calculate the gl_VertexID as
SYSTEM_VALUE_VERTEX_ID_ZERO_BASE plus SYSTEM_VALUE_BASE_VERTEX_ID.
Note that the current calculation which uses SYSTEM_VALUE_BASE_VERTEX
is not right, as gl_BaseVertex should be zero for non-indexed calls.

Reviewed-by: Neil Roberts 
---
 src/compiler/shader_enums.c |  1 +
 src/compiler/shader_enums.h | 15 +++
 2 files changed, 16 insertions(+)

diff --git a/src/compiler/shader_enums.c b/src/compiler/shader_enums.c
index b2ca80b49c2..cd8c256f227 100644
--- a/src/compiler/shader_enums.c
+++ b/src/compiler/shader_enums.c
@@ -215,6 +215,7 @@ gl_system_value_name(gl_system_value sysval)
  ENUM(SYSTEM_VALUE_INSTANCE_ID),
  ENUM(SYSTEM_VALUE_INSTANCE_INDEX),
  ENUM(SYSTEM_VALUE_VERTEX_ID_ZERO_BASE),
+ ENUM(SYSTEM_VALUE_BASE_VERTEX_ID),
  ENUM(SYSTEM_VALUE_BASE_VERTEX),
  ENUM(SYSTEM_VALUE_BASE_INSTANCE),
  ENUM(SYSTEM_VALUE_DRAW_ID),
diff --git a/src/compiler/shader_enums.h b/src/compiler/shader_enums.h
index 9d229d4199e..7437cccdd0a 100644
--- a/src/compiler/shader_enums.h
+++ b/src/compiler/shader_enums.h
@@ -474,6 +474,21 @@ typedef enum
 */
SYSTEM_VALUE_BASE_VERTEX,
 
+
+   /**
+* Depending on the type of the draw call (indexed or non-indexed),
+* is the value of \c basevertex passed to \c glDrawElementsBaseVertex and
+* similar, or is the value of \c first passed to \c glDrawArrays and
+* similar.
+*
+* \note
+* It can be used to calculate the \c SYSTEM_VALUE_VERTEX_ID as
+* \c SYSTEM_VALUE_VERTEX_ID_ZERO_BASE plus \c SYSTEM_VALUE_BASE_VERTEX_ID.
+*
+* \sa SYSTEM_VALUE_VERTEX_ID_ZERO_BASE, SYSTEM_VALUE_VERTEX_ID
+*/
+   SYSTEM_VALUE_BASE_VERTEX_ID,
+
/**
 * Value of \c baseinstance passed to instanced draw entry points
 *
-- 
2.14.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev