[Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/18/2013 03:43 PM, Francisco Jerez wrote:
> Ian Romanick <idr at freedesktop.org> writes:
> 
>> On 09/17/2013 03:51 PM, Paul Berry wrote:
>>> On 15 September 2013 00:10, Francisco Jerez
>>> <currojerez at riseup.net <mailto:currojerez at riseup.net>> wrote: 
>>> [...]
>>> 
>>> GLSL 4.20 clarifies that atomic counter offsets must be unique
>>> and non-overlapping (see GLSL 4.20 4.4.4.1 "Atomic Counter
>>> Layout Qualifiers").  So I tihnk we need a stronger check than
>>> this. Specifically, declarations like the following should be
>>> disallowed:
>>> 
>>> layout(binding=0, offset=16) atomic_uint foo[4]; // Uses
>>> offsets 16, 20, 24, 28 layout(binding=0, offset=20) atomic_uint
>>> bar; // Error: overlaps foo.
>> 
>> I also think this check belongs in the linker.  I don't see a lot
>> of value in doing it twice, and we have to verify atomics
>> declared in other compilation units.
>> 
> I can move that check back to the linker, you're right that it
> would probably be more convenient for us to do so.  The spec isn't
> very clear on whether this should be a compile or a link-time
> error, actually I implemented this in the linker first but in doubt
> it seemed desirable to be able to catch this sort of errors as
> early as possible, and the spec mentions explicitly that using
> atomic buffer bindings over the implementation's maximum should
> cause a compile-time error.

There's two checks.  Right?

Check that binding and offset are within implementation limits.  This
can be done in the compiler or the linker.  I generally prefer earlier.

Check that bindings and offsets of multiple variables do not overlap.
 Since you may link multiple compilation units, it is impossible to
know before the linker because you don't have access to the other
compilation units yet.

>>> 
>>> +         if (field_type->atomic_size()) { +            YYLTYPE
>>> loc = decl_list->get_location(); +
>>> _mesa_glsl_error(&loc, state, "atomic counter in structure or
>>> " +                             "uniform block"); +         } 
>>> +
>>> 
>>> Are you sure this is not allowed?  I can't find any spec text
>>> to back this up.  All I found was this text from
>>> ARB_shader_atomic_counters:
>> 
>> Yeah, this is definitely wrong.  See the text I quoted in reply
>> to patch 8.[...]
>> 
> Please read my answer to Paul's post.  It's not clear how atomic 
> counters declared within a structure would get their binding points
> and offsets assigned: binding and offset qualifiers are disallowed
> within structs, and using a single layout() qualifier for the whole
> uniform struct declaration isn't satisfactory either.  I think we
> should disallow them for now, the spec is just too inconsistent...

Yes... I see the problem now.  I've submitted a spec bug.  Until that
bug gets resolved, let's continue with the assumption that atomic_uint
should not be allowed in structs.

> Thanks.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)

iEYEARECAAYFAlI6HXUACgkQX1gOwKyEAw8iXQCdEg2FXG/b9BmT9apdJdgkHIqO
oj4AoJXMkDW85VDHbxiooTDyT6vAGm/3
=cQ8z
-----END PGP SIGNATURE-----