[Mesa-dev] [PATCH 0/9] Fix shader_draw_parameters CTS tests

[Kenneth Graunke]

On Friday, November 10, 2017 9:53:28 AM PST Antia Puentes wrote:
> Hi,
> 
> the series sets gl_BaseVertex to zero for gl*DrawArrays* in i965.
[snip]

Hi Antia, Neil,

The end result of this series looks reasonable, but it looks like it's
going to break a bunch of things at each step.  Either that or I'm a bit
lost, sorry.

Could you reorganize/split  the patches so that things keep working at
each step along the way?

>   compiler: Add new system value SYSTEM_VALUE_BASE_VERTEX_ID
>   nir: Add SYSTEM_VALUE_BASE_VERTEX_ID instrinsics
>   intel/compiler: Add a uses_basevertexid flag
>   i965: emit basevertexid as a vertex element component

It looks like this moves SYSTEM_VALUE_BASE_VERTEX from VE1 to VE2...but
it doesn't update the compiler to fetch gl_BaseVertex from the new
location.  Plus, we haven't switched to using BASE_VERTEX_ID lowering
for gl_VertexID yet...so prog_data->uses_basevertexid will never be set,
so we won't upload BASE_VERTEX_ID either.

It sure looks like this would break anything using gl_VertexID or
gl_BaseVertex.  Maybe I'm missing something.

>   i965: gl_BaseVertex must be zero for non-indexed draw calls

Again, at this point, we're still using SYSTEM_VALUE_BASE_VERTEX for
gl_VertexID lowering, so this would break gl_VertexID tests.

>   intel/compiler: implement the basevertex(id) load intrinsics
>   nir: Offset vertex_id by base_vertex_id instead of base_vertex
>   i965: Let nir lower gl_VertexID instead of the linker
>   spirv: Lower BaseVertex to BASE_VERTEX_ID instead of BASE_VERTEX

The order I'd expect to see is:

1. Introduce new SYSTEM_VALUE_FIRST_VERTEX enum and nir intrinsics.

2. Add vs_prog_data::uses_first_vertex or system_values_read field.

3. In Intel code, move system values around to new VE locations.

   Vertex Element 1: <Base Vertex, blank/garbage, Vertex ID, Instance ID>
   Vertex Element 2: <Base Instance, Draw ID, 0, 0>

   This would involve changing intel/compiler and both brw and anv's
   state upload code, in the same patch.

4. In Intel code, start uploading SYSTEM_VALUE_FIRST_VERTEX as VE1.y.
   Update the state upload code and the compiler in the same patch.

   Now all the values are in place, but nothing uses the new one yet.

5. Switch to NIR instead of GLSL IR based Vertex ID lowering.
   (i965: Let nir lower gl_VertexID instead of the linker)

6. Make NIR VertexID lowering use SYSTEM_VALUE_FIRST_VERTEX.
   (nir: Offset vertex_id by base_vertex_id instead of base_vertex)

   Now i965 no longer uses SYSTEM_VALUE_BASE_VERTEX for VertexID,
   so you can safely change the meaning.

7. Fix gl_BaseVertex to be 0 for non-indexed draw calls.
   (i965: gl_BaseVertex must be zero for non-indexed draw calls)

Neil, in case you were wondering, I suggested the above organization
of vertex elements because it would let us only upload 1 in the common
case.  Looking in shader-db, there are 3579 shaders that use
gl_InstanceID, 186 shaders that use gl_VertexID, and 0 shaders that use
gl_BaseVertex, gl_BaseInstance, or gl_DrawID.

It looks like your patches kicked gl_BaseVertex off to VE2 instead of
gl_BaseInstance.  I suppose that works too, I just figured that keeping
VertexID/FirstVertex/BaseVertex together would make sense.  If it's more
convenient to move gl_BaseVertex, I suppose I'm fine with that too...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171130/50c86dec/attachment.sig>