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

[Juan A. Suarez Romero]

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? As said, it is fixing a crash when running a CTS
test.


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