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

[Timothy Arceri]

On Fri, 2016-02-12 at 18:45 -0500, Ilia Mirkin wrote:
> On Fri, Feb 12, 2016 at 5:53 PM, Timothy Arceri <t_arceri at yahoo.com.a
> u> 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");
> >    }
> 
> Nope, that doesn't work. In this case
> 
> layout(location = 3, binding = 0, offset = 0)
> 
> it goes into the if () case above, as these are all merged by the
> time
> it goes into hir. Also, it's legal to just have binding, in which
> case
> it'd go into the else, which we don't want either.
> 
> This has to be a standalone condition if I do it here.

Right. I corrected myself on IRC. IMO it should be done something like
this.

   /* 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");
   
As I don't see why we should be treating location as a special case.


> 
> An alternative, as mentioned on IRC, is to stick it into
> apply_explicit_location in the if (uniform) check. Or maybe somewhere
> else entirely.



> 
>   -ilia