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

[Juan A. Suarez Romero]

On Mon, 2017-07-10 at 12:18 +0200, 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.
> > 
> 
> Not sure, could be. I've run piglit and CTS tests looking up for
> regressions, but found nothing.
> 
> > 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.
> > 
> 
> 
> Any other feedback on this is welcomed.
> 
> 

Any other opinion on this?


	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
> > > > 
> > > > 
> > 
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev