[Mesa-dev] [PATCH] glsl: atomic counters are different than their uniforms
Ian Romanick
idr at freedesktop.org
Thu Jun 30 07:29:38 UTC 2016
On 06/29/2016 05:55 PM, Timothy Arceri wrote:
> On Mon, 2016-06-27 at 18:28 +0300, Andres Gomez wrote:
>> The linker deals with atomic counters in terms of uniforms. This is
>> OK
>> but when we want to know the number of used atomic counters since a 2
>> elements atomic counters array will use 2 counters but only 1
>> uniform.
>
> You don't really mention what you are changing. How about:
>
> glsl: count atomic counters correctly.
>
> Currently the linker uses the uniform count for the total number of
> atomic counters. However uniforms don't include the innermost array
> dimension in their count, but atomic counters are expected to include
> them.
>
> <insert supporting spec quote??>
So, I don't think there is a clear part of the spec to quote here, and
I looked. :) While the spec doesn't come right out and say it, I think
we can infer that this behavior is correct. It's clear how offsets
will be assigned for arrays:
* Arrays of type atomic_uint are stored in memory by element order,
with array element member zero at the lowest offset. The difference
in offsets between each pair of elements in the array in basic machine
units is referred to as the array stride, and is constant across the
entire array. The stride can be queried by calling GetIntegerv with
a <pname> of UNIFORM_ARRAY_STRIDE after a program is linked.
>From that it is clear how arrays of atomic counters will interact with
GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE.
For other kinds of uniforms it's also clear that each entry in an array
counts against the relevant limits.
I think this is correct, but you have to rely on inference.
> Fixes GL44-CTS.arrays_of_arrays_gl.AtomicDeclaration
>
>>
>> Renamed the data structures used in the linker for disambiguation.
>
> You should really have split this in two. One patch to rename things
> and one to change make the actual change.
>
> If you fix up the commit message then you can add:
>
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
>
> I'd also rather you split this into two patches before pushing but I
> can live with it if you don't.
>
> Thanks,
> Tim
>
>>
>> Fixes GL44-CTS.arrays_of_arrays_gl.AtomicDeclaration
>>
>> Signed-off-by: Andres Gomez <agomez at igalia.com>
>> ---
>> src/compiler/glsl/link_atomics.cpp | 83 ++++++++++++++++++++------
>> ------------
>> 1 file changed, 44 insertions(+), 39 deletions(-)
>>
>> diff --git a/src/compiler/glsl/link_atomics.cpp
>> b/src/compiler/glsl/link_atomics.cpp
>> index 277d473..10c3c1a 100644
>> --- a/src/compiler/glsl/link_atomics.cpp
>> +++ b/src/compiler/glsl/link_atomics.cpp
>> @@ -30,9 +30,9 @@
>>
>> namespace {
>> /*
>> - * Atomic counter as seen by the program.
>> + * Atomic counter uniform as seen by the program.
>> */
>> - struct active_atomic_counter {
>> + struct active_atomic_counter_uniform {
>> unsigned uniform_loc;
>> ir_variable *var;
>> };
>> @@ -44,44 +44,44 @@ namespace {
>> */
>> struct active_atomic_buffer {
>> active_atomic_buffer()
>> - : counters(0), num_counters(0), stage_references(), size(0)
>> + : uniforms(0), num_uniforms(0), stage_counter_references(),
>> size(0)
>> {}
>>
>> ~active_atomic_buffer()
>> {
>> - free(counters);
>> + free(uniforms);
>> }
>>
>> void push_back(unsigned uniform_loc, ir_variable *var)
>> {
>> - active_atomic_counter *new_counters;
>> + active_atomic_counter_uniform *new_uniforms;
>>
>> - new_counters = (active_atomic_counter *)
>> - realloc(counters, sizeof(active_atomic_counter) *
>> - (num_counters + 1));
>> + new_uniforms = (active_atomic_counter_uniform *)
>> + realloc(uniforms, sizeof(active_atomic_counter_uniform)
>> *
>> + (num_uniforms + 1));
>>
>> - if (new_counters == NULL) {
>> + if (new_uniforms == NULL) {
>> _mesa_error_no_memory(__func__);
>> return;
>> }
>>
>> - counters = new_counters;
>> - counters[num_counters].uniform_loc = uniform_loc;
>> - counters[num_counters].var = var;
>> - num_counters++;
>> + uniforms = new_uniforms;
>> + uniforms[num_uniforms].uniform_loc = uniform_loc;
>> + uniforms[num_uniforms].var = var;
>> + num_uniforms++;
>> }
>>
>> - active_atomic_counter *counters;
>> - unsigned num_counters;
>> - unsigned stage_references[MESA_SHADER_STAGES];
>> + active_atomic_counter_uniform *uniforms;
>> + unsigned num_uniforms;
>> + unsigned stage_counter_references[MESA_SHADER_STAGES];
>> unsigned size;
>> };
>>
>> int
>> cmp_actives(const void *a, const void *b)
>> {
>> - const active_atomic_counter *const first =
>> (active_atomic_counter *) a;
>> - const active_atomic_counter *const second =
>> (active_atomic_counter *) b;
>> + const active_atomic_counter_uniform *const first =
>> (active_atomic_counter_uniform *) a;
>> + const active_atomic_counter_uniform *const second =
>> (active_atomic_counter_uniform *) b;
>>
>> return int(first->var->data.offset) - int(second->var-
>>> data.offset);
>> }
>> @@ -103,9 +103,9 @@ namespace {
>> const unsigned shader_stage)
>> {
>> /* FIXME: Arrays of arrays get counted separately. For
>> example:
>> - * x1[3][3][2] = 9 counters
>> - * x2[3][2] = 3 counters
>> - * x3[2] = 1 counter
>> + * x1[3][3][2] = 9 uniforms, 18 atomic counters
>> + * x2[3][2] = 3 uniforms, 6 atomic counters
>> + * x3[2] = 1 uniforms, 2 atomic counters
>> *
>> * However this code marks all the counters as active even
>> when they
>> * might not be used.
>> @@ -129,7 +129,12 @@ namespace {
>>
>> buf->push_back(*uniform_loc, var);
>>
>> - buf->stage_references[shader_stage]++;
>> + /* When checking for atomic counters we should count every
>> member in
>> + * an array as an atomic counter reference. */
>> + if (t->is_array())
>> + buf->stage_counter_references[shader_stage] += t-
>>> length;
>> + else
>> + buf->stage_counter_references[shader_stage]++;
>> buf->size = MAX2(buf->size, *offset + t->atomic_size());
>>
>> storage->offset = *offset;
>> @@ -170,22 +175,22 @@ namespace {
>> if (buffers[i].size == 0)
>> continue;
>>
>> - qsort(buffers[i].counters, buffers[i].num_counters,
>> - sizeof(active_atomic_counter),
>> + qsort(buffers[i].uniforms, buffers[i].num_uniforms,
>> + sizeof(active_atomic_counter_uniform),
>> cmp_actives);
>>
>> - for (unsigned j = 1; j < buffers[i].num_counters; j++) {
>> + for (unsigned j = 1; j < buffers[i].num_uniforms; j++) {
>> /* If an overlapping counter found, it must be a
>> reference to the
>> * same counter from a different shader stage.
>> */
>> - if (check_atomic_counters_overlap(buffers[i].counters[j-
>> 1].var,
>> - buffers[i].counters[j]
>> .var)
>> - && strcmp(buffers[i].counters[j-1].var->name,
>> - buffers[i].counters[j].var->name) != 0) {
>> + if (check_atomic_counters_overlap(buffers[i].uniforms[j-
>> 1].var,
>> + buffers[i].uniforms[j]
>> .var)
>> + && strcmp(buffers[i].uniforms[j-1].var->name,
>> + buffers[i].uniforms[j].var->name) != 0) {
>> linker_error(prog, "Atomic counter %s declared at
>> offset %d "
>> "which is already in use.",
>> - buffers[i].counters[j].var->name,
>> - buffers[i].counters[j].var-
>>> data.offset);
>> + buffers[i].uniforms[j].var->name,
>> + buffers[i].uniforms[j].var-
>>> data.offset);
>> }
>> }
>> }
>> @@ -223,16 +228,16 @@ link_assign_atomic_counter_resources(struct
>> gl_context *ctx,
>> mab.Binding = binding;
>> mab.MinimumSize = ab.size;
>> mab.Uniforms = rzalloc_array(prog->AtomicBuffers, GLuint,
>> - ab.num_counters);
>> - mab.NumUniforms = ab.num_counters;
>> + ab.num_uniforms);
>> + mab.NumUniforms = ab.num_uniforms;
>>
>> /* Assign counter-specific fields. */
>> - for (unsigned j = 0; j < ab.num_counters; j++) {
>> - ir_variable *const var = ab.counters[j].var;
>> + for (unsigned j = 0; j < ab.num_uniforms; j++) {
>> + ir_variable *const var = ab.uniforms[j].var;
>> gl_uniform_storage *const storage =
>> - &prog->UniformStorage[ab.counters[j].uniform_loc];
>> + &prog->UniformStorage[ab.uniforms[j].uniform_loc];
>>
>> - mab.Uniforms[j] = ab.counters[j].uniform_loc;
>> + mab.Uniforms[j] = ab.uniforms[j].uniform_loc;
>> if (!var->data.explicit_binding)
>> var->data.binding = i;
>>
>> @@ -246,7 +251,7 @@ link_assign_atomic_counter_resources(struct
>> gl_context *ctx,
>>
>> /* Assign stage-specific fields. */
>> for (unsigned j = 0; j < MESA_SHADER_STAGES; ++j) {
>> - if (ab.stage_references[j]) {
>> + if (ab.stage_counter_references[j]) {
>> mab.StageReferences[j] = GL_TRUE;
>> num_atomic_buffers[j]++;
>> } else {
>> @@ -314,7 +319,7 @@ link_check_atomic_counter_resources(struct
>> gl_context *ctx,
>> continue;
>>
>> for (unsigned j = 0; j < MESA_SHADER_STAGES; ++j) {
>> - const unsigned n = abs[i].stage_references[j];
>> + const unsigned n = abs[i].stage_counter_references[j];
>>
>> if (n) {
>> atomic_counters[j] += n;
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list