On Fri, Feb 12, 2016 at 8:52 PM, Timothy Arceri <t_arc...@yahoo.com.au> wrote:
> On Fri, 2016-02-12 at 18:45 -0500, Ilia Mirkin wrote:
>> On Fri, Feb 12, 2016 at 5:53 PM, Timothy Arceri <t_arc...@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 <imir...@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.
Ah OK. But this should be done here, or moved into
ast_type::merge_qualifier or whereever it does the other masking
logic?
-ilia
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
