[Mesa-dev] [PATCH] i965: skip varyings without slot

[Timothy Arceri]

On 27/06/17 21:20, Juan A. Suarez Romero wrote:
> On Tue, 2017-06-27 at 09:29 +1000, Timothy Arceri wrote:
>> On 16/06/17 18:12, Juan A. Suarez Romero wrote:
>>
>>> Commit 00620782c9 (i965: use nir_shader_gather_info() over
>>> do_set_program_inouts()) changed how we compute the outputs written.
>>>
>>> In the previous version it was using the IR declared outputs, while in
>>> the new one it uses NIR to parse the instructions that write outputs.
>>>
>>> Thus, if the shader has declared some output that is not written later
>>> in the code, like this:
>>>
>>> ~~~
>>> struct S {
>>>       vec4 a;
>>>       vec4 b;
>>>       vec4 c;
>>> };
>>>
>>> layout (xfb_offset = sizeof_type) out S s;
>>>
>>> void main()
>>> {
>>>
>>>       s.a = vec4(1.0, 0.0, 0.0, 1.0);
>>>       s.c = vec4(0.0, 1.0, 0.0, 1.0);
>>> }
>>> ~~~
>>>
>>> The former version computing 3 outputs written (s.a, s.b and s.c), while
>>> the new version only counts 2 (s.a and s.c).
>>>
>>> This means that with the new version, then could be varyings in the VUE
>>> map that do not have an slot assigned (s.b), that must be skipped.
>>>
>>> This fixes KHR-GL45.enhanced_layouts.xfb_capture_struct.
>>> ---
>>>    src/mesa/drivers/dri/i965/genX_state_upload.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
>>> index a5ad2ca..573f0e3 100644
>>> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
>>> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
>>> @@ -3102,9 +3102,10 @@ genX(upload_3dstate_so_decl_list)(struct brw_context *brw,
>>>          const unsigned stream_id = output->StreamId;
>>>          assert(stream_id < MAX_VERTEX_STREAMS);
>>>    
>>> -      buffer_mask[stream_id] |= 1 << buffer;
>>> +      if (vue_map->varying_to_slot[varying] == -1)
>>> +	      continue;
>>>    
>>> -      assert(vue_map->varying_to_slot[varying] >= 0);
>>> +      buffer_mask[stream_id] |= 1 << buffer;
>>>    
>>
>> My feeling is we should try to avoid adding it to the VUE map in the
>> first place rather than trying to work around it.
>>
> 
> It isn't in the VUE map. That's the reason to skip it.
> 
> Maybe you mean not adding it in the linked_xfb_info?

oh, right. I had it the wrong way around in my head.

I think the problem is we setup xfb in the glsl linker but then run all 
the NIR optimisation before calling nir_shader_gather_info().

However I'm not sure removing the assert is the best idea, as it could 
result in real issues being hidden.

Ideally we would run the NIR opts before we do the final linking in GLSL 
IR. I've outlined how this can be done in past emails (which I can't 
seem to find), but its a lot of work. Nicolai's spirv might make is 
easier to do, but there will still be things like a nir varying packing 
pass required which I believe will be outside of what Nicolai needs for 
his changes.

For now I believe this issue only impacts debug builds so I'm not sure 
removing the assert and silently skipping is a good idea.

I'll let others comment further.

> 
> 	J.A.
> 
> 
> 
>> Is it not possible to do that instead?
>>
>>
>>>          /* Mesa doesn't store entries for gl_SkipComponents in the Outputs[]
>>>           * array.  Instead, it simply increments DstOffset for the following
>>
>>