[Mesa-dev] [PATCH] glsl: reject explicit location on atomic counter uniforms

[Timothy Arceri]

On Mon, 2016-02-15 at 10:49 -0500, Nicolai Hähnle wrote:
> 
> On 12.02.2016 17:53, Timothy Arceri wrote:
> > On Thu, 2016-02-11 at 20:10 -0500, Ilia Mirkin wrote:
> > > This fixes
> > > 
> > > dEQP-GLES31.functional.uniform_location.negative.atomic_fragment
> > > dEQP-GLES31.functional.uniform_location.negative.atomic_vertex
> > > 
> > > Both of which have lines like
> > > 
> > > layout(location = 3, binding = 0, offset = 0) uniform atomic_uint
> > > uni0;
> > > 
> > > The ARB_explicit_uniform_location spec makes a very tangential
> > > mention
> > > regarding atomic counters, but location isn't something that
> > > makes
> > > sense
> > > with them.
> > > 
> > > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > > ---
> > > 
> > > Had no clue where to stick this check... this seemed like as good
> > > a
> > > place as any.
> > > 
> > >   src/compiler/glsl/ast_to_hir.cpp | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> > > b/src/compiler/glsl/ast_to_hir.cpp
> > > index dbeb5c0..9fce06b 100644
> > > --- a/src/compiler/glsl/ast_to_hir.cpp
> > > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > > @@ -4179,6 +4179,11 @@ ast_declarator_list::hir(exec_list
> > > *instructions,
> > >               state->atomic_counter_offsets[qual_binding] =
> > > qual_offset;
> > >            }
> > >         }
> > 
> > Maybe we should just make this:
> >     else {
> >        _mesa_glsl_error(&loc, state, "invalid atomic counter layout
> > qualifier");
> >     }
> > 
> > ??
> 
> FWIW, I like Ilia's original message better because it gives the
> user 
> more information about why exactly their layout qualifier is
> invalid. 
> Helpful error messages are a good thing.

Sure I agree. The complete solution would be to use a mask for
validation.

    /* Valid atomic layout qualifiers */
    ast_type_qualifier atomic_layout_mask;
    atomic_layout_mask.flags.i = 0;
    atomic_layout_mask.flags.q.explicit_binding = 1;
    atomic_layout_mask.flags.q.explicit_offset = 1;
    atomic_layout_mask.flags.q.unifrom = 1 ;
    ...

    if ((qual->flags.i & ~atomic_layout_mask.flags.i) != 0)
       _mesa_glsl_error(&loc, state, "invalid atomic counter layout
 qualifier");

This would allow catching of all invalid qualifiers rather than just
one off checks which is going to get messy fast. It's likely we can
remove some existing code for various types by implementing something
like this.

We could then have a generic helper that turns the check into a more
useful message based on the flags. This would be useful for exiting
checks as this is how we validate global input qualifiers and I've just
sent a patch to do it on varying inputs.

https://patchwork.freedesktop.org/patch/73710/

Personnaly I'd rather this deqp test keep failing rather than add a one
off case to catch location.

> 
> That said, I won't complain too loudly if making the code simpler or 
> easier to follow ends up making the error messages slightly less
> helpful.
> 
> Cheers,
> Nicolai
> 
> > 
> > > +
> > > +      if (type->qualifier.flags.q.explicit_location) {
> > > +         _mesa_glsl_error(&loc, state,
> > > +                          "atomic counters cannot have an
> > > explicit
> > > location");
> > > +      }
> > >      }
> > > 
> > >      if (this->declarations.is_empty()) {
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >