[Mesa-dev] [PATCH] i965: skip varyings without slot
Juan A. Suarez Romero
jasuarez at igalia.com
Mon Jul 10 10:18:42 UTC 2017
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.
> >
> > 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
> > >
> > >
>
>
More information about the mesa-dev
mailing list