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

[Juan A. Suarez Romero]

On Wed, 2017-09-06 at 10:32 +1000, Timothy Arceri wrote:
> 
> 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.
> 


Kenneth provided a different approach to fix this issue.

https://patchwork.freedesktop.org/series/30439/

	J.A.