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

[Francisco Jerez]

Paul Berry <stereotype441 at gmail.com> writes:

> On 15 September 2013 00:10, Francisco Jerez <currojerez at riseup.net> wrote:
>[...]
>> +      } else if ((op[0]->type->atomic_size() ||
>> op[1]->type->atomic_size())) {
>> +        _mesa_glsl_error(&loc, state, "atomic counter comparisons
>> forbidden");
>> +        error_emitted = true;
>>
>
> Do we have spec text to back this up?  I looked around and couldn't find
> anything.  It seems like doing equality comparisons on atomic counters is
> ill-defined, though (do two counters compare equal if they have equal
> values, or if they point to the same counter?  Both possibilities seem
> dodgy).  Maybe we should file a spec bug so we can get clarification from
> khronos about what's intended.
>

It's implied by the same wording you quoted below, "Except for array
indexing, structure field selection, and parenthesis, counters are not
allowed to be operands in expressions.".

> Note that we currently permit other comparisons that are similarly dodgy
> (e.g. comparing samplers).  It would be nice to get clarification from
> khronos about this too.
>
>[...]
>> @@ -1983,7 +1995,7 @@ apply_type_qualifier_to_variable(const struct
>> ast_type_qualifier *qual,
>>     }
>>
>>     if (qual->flags.q.constant || qual->flags.q.attribute
>> -       || qual->flags.q.uniform
>> +       || (qual->flags.q.uniform && var->type !=
>> glsl_type::atomic_uint_type)
>>         || (qual->flags.q.varying && (state->target == fragment_shader)))
>>        var->read_only = 1;
>>
>
> I'm not entirely convinced this is right.  An atomic counter, like a
> sampler, is a handle to an underlying object, not the underlying object
> itself.  The handle should be considered read-only even if the object it
> refers to is mutable.  That way we still prohibit
>
I guess the question is if we want atomic ir_variables represent the
handle of an atomic counter or the atomic counter itself.  The latter
seemed to make more sense to me but you're right that the spec favours
the opposite point of view, I will fix that.

> uniform atomic_uint foo;
> uniform atomic_uint bar;
> void main() {
>    foo = bar;
> }
>
>
>>[...]
>> @@ -4475,6 +4533,12 @@ ast_process_structure_or_interface_block(exec_list
>> *instructions,
>>                               "uniform in non-default uniform block
>> contains sampler");
>>           }
>>
>> +         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:
>
> "Except for array indexing, structure field selection, and parenthesis,
> counters are not allowed to be operands in expressions."
>
> Which would seem to imply that atomic counters *are* allowed as structure
> fields.
>

That's a tricky question, my impression when I first read this extension
was that it was allowed, but the spec seemed to be inconsistent in that
regard and it leaves a number of questions unanswered.

The wording you're quoting doesn't apply directly to atomic counters as
they are defined by the GL 4.2 spec, it refers to opaque types in
general:

"Except for array indexing, structure member selection, and parentheses,
 opaque variables are not allowed to be operands in expressions; such
 use results in a compile-time error."

Later on the spec requires atomic counter uniform declarations to have a
layout() specification tying them to some specific binding point, which
seems to be forbidden in structure member declarations:

"[Structure] member declarators may contain precision qualifiers, but
 use of any other qualifier results in a compile-time error."

One could imagine that the structure declaration syntax could be relaxed
in cases where a structure contains atomic counters to allow binding and
offset qualifiers on the whole structure, like:

| struct S {
|   ....
|   atomic_uint x;
| };
|
| layout(binding=X, offset=Y) uniform S foo;
|

The spec doesn't specify this syntax nor how atomic counters contained
in the structure would be supposed to get their bindings and offsets
assigned.  It seems it would be problematic because of the way the
binding and offset qualifiers have different meanings for different type
declarations -- E.g. what if you declare a structure containing an
atomic counter and an image2D?  Are they supposed to get the same
binding even though they refer to completely different name spaces?  Is
the binding number supposed to be post-incremented for each declaration
like is the case for image and sampler arrays, or stay constant as in
atomic counter arrays?

To summarize, the spec doesn't seem to provide any reasonable way to
declare structures with atomic counters even though it provides the
syntax for atomic structure member selection...

For uniform blocks OTOH it's quite clear that they are not allowed:

"[In interface blocks] types and declarators are the same as for other
input, output, uniform, and buffer variable declarations outside blocks,
with these exceptions: [...] opaque types are not allowed."

Thanks.

>
>>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130917/cc2c56fd/attachment.pgp>