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

[Timothy Arceri]

On 01/09/17 21:15, Juan A. Suarez Romero wrote:
> On Thu, 2017-06-29 at 14:43 +1000, Timothy Arceri wrote:
>> 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.
>>
> 
> 
> After couple of months, didn't get any other feedback.
> 
> Should this be R-b?

No.

> As said, it is fixing a crash when running a CTS
> test.

As I've said above the crash is unfortunate but it's a false positive 
that doesn't impact the release build what so ever (please correct me if 
this is wrong). The assert will however catch real issues as well that 
your patch would just ignore so I'd rather just leave it as is.

If we really want to fix this then we should run the NIR opts before we 
do the final linking in GLSL IR as described above, however there are 
some significant outstanding tasks that need to be completed before that 
can be done. For example we need a nir varying packing pass.

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