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...
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