[Mesa-dev] [PATCH 13/24] glsl: Linker support for ARB_shader_atomic_counters.
Paul Berry
stereotype441 at gmail.com
Thu Sep 19 10:48:09 PDT 2013
On 15 September 2013 00:10, Francisco Jerez <currojerez at riseup.net> wrote:
> ---
> src/glsl/Makefile.sources | 1 +
> src/glsl/link_atomics.cpp | 190
> ++++++++++++++++++++++++++++++++++++++++++++++
> src/glsl/linker.cpp | 15 ++++
> src/glsl/linker.h | 7 ++
> 4 files changed, 213 insertions(+)
> create mode 100644 src/glsl/link_atomics.cpp
>
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 2f7bfa1..197d081 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -47,6 +47,7 @@ LIBGLSL_FILES = \
> $(GLSL_SRCDIR)/ir_validate.cpp \
> $(GLSL_SRCDIR)/ir_variable_refcount.cpp \
> $(GLSL_SRCDIR)/linker.cpp \
> + $(GLSL_SRCDIR)/link_atomics.cpp \
> $(GLSL_SRCDIR)/link_functions.cpp \
> $(GLSL_SRCDIR)/link_interface_blocks.cpp \
> $(GLSL_SRCDIR)/link_uniforms.cpp \
> diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
> new file mode 100644
> index 0000000..a623454
> --- /dev/null
> +++ b/src/glsl/link_atomics.cpp
> @@ -0,0 +1,190 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <vector>
> +#include <map>
> +
> +#include "ir.h"
> +#include "ir_uniform.h"
> +#include "linker.h"
> +#include "program/hash_table.h"
> +
> +namespace {
> + struct active_atomic_counter {
> + active_atomic_counter(unsigned id, ir_variable *var) :
> + id(id), var(var) {}
> +
> + unsigned id;
> + ir_variable *var;
> + };
> +
> + typedef std::vector<active_atomic_counter> active_atomic_counters_t;
> +
> + struct active_atomic_buffer {
> + active_atomic_buffer() : stage_references(), size(0) {}
> +
> + active_atomic_counters_t counters;
> + unsigned stage_references[MESA_SHADER_TYPES];
>
+ unsigned size;
> + };
> +
> + typedef std::map<unsigned, active_atomic_buffer>
> active_atomic_buffers_t;
>
It would be really helpful to have some comments above these structs and
their members indicating how they relate to the constructs in the language.
Also, if I'm understanding correctly, these data structures don't actually
represent buffers, but rather represent binding points that buffers will
later be bound to, so I'd also recommend renaming active_atomic_buffer ->
active_atomic_binding_point and active_atomic_buffers_t ->
active_atomic_binding_points_t, to be more in line with the language used
in the spec.
> +
> + /**
> + * Construct a collection of active_atomic_buffer structures
> + * indexed by binding point. Each entry includes a collection of
> + * active_atomic_counters that represent the set of atomic counters
> + * determined to be contained within the same buffer.
> + */
> + active_atomic_buffers_t
> + find_active_atomic_counters(struct gl_shader_program *prog) {
> + active_atomic_buffers_t abs;
> +
> + for (unsigned i = 0; i < MESA_SHADER_TYPES; ++i) {
> + struct gl_shader *sh = prog->_LinkedShaders[i];
> + if (sh == NULL)
> + continue;
> +
> + foreach_list(node, sh->ir) {
> + ir_variable *var = ((ir_instruction *)node)->as_variable();
> +
> + if (var && var->type->atomic_size()) {
> + unsigned id;
> + bool found = prog->UniformHash->get(id, var->name);
> + assert(found);
> + active_atomic_buffer &ab = abs[var->binding];
> +
> + ab.counters.push_back(active_atomic_counter(id, var));
>
Are uniform id's unique across program stages? If not, it seems like we
are losing information here.
> + ab.stage_references[i]++;
> + ab.size = std::max(ab.size, var->atomic.offset +
> + var->type->atomic_size());
> + }
> + }
> + }
> +
> + return abs;
> + }
> +}
> +
> +void
> +link_assign_atomic_counter_resources(struct gl_shader_program *prog)
> +{
> + active_atomic_buffers_t abs = find_active_atomic_counters(prog);
> +
> + prog->AtomicBuffers = rzalloc_array(prog, gl_active_atomic_buffer,
> + abs.size());
> + prog->NumAtomicBuffers = abs.size();
>
Similar to my comments above, I'd recommend renaming these to
prog->AtomicBindingPoints and prog->NumAtomicBindingPoints.
> +
> + unsigned i = 0;
> + for (active_atomic_buffers_t::iterator it = abs.begin();
> + it != abs.end(); ++it, ++i) {
> + active_atomic_buffer &ab = it->second;
> + gl_active_atomic_buffer &mab = prog->AtomicBuffers[i];
> +
> + /* Assign buffer-specific fields. */
> + mab.Binding = it->first;
> + mab.MinimumSize = ab.size;
> + mab.Uniforms = rzalloc_array(prog->AtomicBuffers, GLuint,
> + ab.counters.size());
> + mab.NumUniforms = ab.counters.size();
> +
> + /* Assign counter-specific fields. */
> + unsigned j = 0;
> + for (active_atomic_counters_t::iterator jt = ab.counters.begin();
> + jt != ab.counters.end(); ++jt, ++j) {
> + ir_variable *var = jt->var;
> + gl_uniform_storage &storage = prog->UniformStorage[jt->id];
> +
> + mab.Uniforms[j] = jt->id;
> + var->atomic.buffer_index = i;
> + storage.atomic_buffer_index = i;
> + storage.offset = var->atomic.offset;
> + storage.array_stride = (var->type->is_array() ?
> + var->type->element_type()->atomic_size()
> : 0);
> + }
> +
> + /* Assign stage-specific fields. */
> + for (j = 0; j < MESA_SHADER_TYPES; ++j)
> + mab.StageReferences[j] =
> + (ab.stage_references[j] ? GL_TRUE : GL_FALSE);
> + }
> +}
> +
> +void
> +link_check_atomic_counter_resources(struct gl_context *ctx,
> + struct gl_shader_program *prog)
> +{
>
Since this function relies so heavily on the assumption that the three
shader types are vertex, geometry, and fragment, can we add a:
STATIC_ASSERT(MESA_SHADER_TYPES == 3);
here? That way in the future when we add tessellation shaders we won't
forget to fix it.
> + static const char *shader_names[MESA_SHADER_TYPES] = {
> + "vertex", "geometry", "fragment"
> + };
> + const unsigned max_atomic_counters[MESA_SHADER_TYPES] = {
> + ctx->Const.VertexProgram.MaxAtomicCounters,
> + ctx->Const.GeometryProgram.MaxAtomicCounters,
> + ctx->Const.FragmentProgram.MaxAtomicCounters
> + };
> + const unsigned max_atomic_buffers[MESA_SHADER_TYPES] = {
> + ctx->Const.VertexProgram.MaxAtomicBuffers,
> + ctx->Const.GeometryProgram.MaxAtomicBuffers,
> + ctx->Const.FragmentProgram.MaxAtomicBuffers
> + };
> + active_atomic_buffers_t abs = find_active_atomic_counters(prog);
> + unsigned atomic_counters[MESA_SHADER_TYPES] = {};
> + unsigned atomic_buffers[MESA_SHADER_TYPES] = {};
> + unsigned total_atomic_counters = 0;
> + unsigned total_atomic_buffers = 0;
> +
> + /* Sum the required resources. Note that this counts buffers and
> + * counters referenced by several shader stages multiple times
> + * against the combined limit -- That's the behavior the spec
> + * requires. */
>
Style nit-pick: for multi-line comments we put the trailing "*/" on a line
by itself.
With those minor issues addressed, the patch is:
Reviewed-by: Paul Berry <stereotype441 at gmail.com>
> + for (active_atomic_buffers_t::iterator it = abs.begin();
> + it != abs.end(); ++it) {
> + for (unsigned j = 0; j < MESA_SHADER_TYPES; ++j) {
> + const unsigned n = it->second.stage_references[j];
> +
> + if (n) {
> + atomic_counters[j] += n;
> + total_atomic_counters += n;
> + atomic_buffers[j]++;
> + total_atomic_buffers++;
> + }
> + }
> + }
> +
> + /* Check that they are within the supported limits. */
> + for (unsigned i = 0; i < MESA_SHADER_TYPES; i++) {
> + if (atomic_counters[i] > max_atomic_counters[i])
> + linker_error(prog, "Too many %s shader atomic counters",
> + shader_names[i]);
> +
> + if (atomic_buffers[i] > max_atomic_buffers[i])
> + linker_error(prog, "Too many %s shader atomic counter buffers",
> + shader_names[i]);
> + }
> +
> + if (total_atomic_counters > ctx->Const.MaxCombinedAtomicCounters)
> + linker_error(prog, "Too many combined atomic counters");
> +
> + if (total_atomic_buffers > ctx->Const.MaxCombinedAtomicBuffers)
> + linker_error(prog, "Too many combined atomic buffers");
> +}
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 8a143fd..c317f46 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -668,6 +668,14 @@ cross_validate_globals(struct gl_shader_program *prog,
> existing->explicit_binding = true;
> }
>
> + if (var->type->atomic_size() &&
> + var->atomic.offset != existing->atomic.offset) {
> + linker_error(prog, "offset specifications for %s "
> + "`%s' have differing values\n",
> + mode_string(var), var->name);
> + return;
> + }
> +
> /* Validate layout qualifiers for gl_FragDepth.
> *
> * From the AMD/ARB_conservative_depth specs:
> @@ -1857,6 +1865,10 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> prog->UniformBlockStageIndex[i] = NULL;
> }
>
> + ralloc_free(prog->AtomicBuffers);
> + prog->AtomicBuffers = NULL;
> + prog->NumAtomicBuffers = 0;
> +
> /* Separate the shaders into groups based on their type.
> */
> struct gl_shader **vert_shader_list;
> @@ -2211,9 +2223,12 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
>
> update_array_sizes(prog);
> link_assign_uniform_locations(prog);
> + link_assign_atomic_counter_resources(prog);
> store_fragdepth_layout(prog);
>
> check_resources(ctx, prog);
> + link_check_atomic_counter_resources(ctx, prog);
> +
> if (!prog->LinkStatus)
> goto done;
>
> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> index 8a0027d..893294a 100644
> --- a/src/glsl/linker.h
> +++ b/src/glsl/linker.h
> @@ -70,6 +70,13 @@ validate_interstage_interface_blocks(struct
> gl_shader_program *prog,
> const gl_shader *producer,
> const gl_shader *consumer);
>
> +extern void
> +link_assign_atomic_counter_resources(struct gl_shader_program *prog);
> +
> +extern void
> +link_check_atomic_counter_resources(struct gl_context *ctx,
> + struct gl_shader_program *prog);
> +
> /**
> * Class for processing all of the leaf fields of a variable that
> corresponds
> * to a program resource.
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130919/fd741ff2/attachment-0001.html>
More information about the mesa-dev
mailing list