<div dir="ltr">On 15 September 2013 00:10, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">--- 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]; </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + unsigned size; + }; + + typedef std::map<unsigned, active_atomic_buffer> active_atomic_buffers_t; </blockquote><div> </div><div>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. </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + /** + * 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)); </blockquote><div> </div><div>Are uniform id's unique across program stages? If not, it seems like we are losing information here. </div> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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(); </blockquote><div> </div><div>Similar to my comments above, I'd recommend renaming these to prog->AtomicBindingPoints and prog->NumAtomicBindingPoints. </div> <div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + 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) +{ </blockquote><div> </div><div>Since this function relies so heavily on the assumption that the three shader types are vertex, geometry, and fragment, can we add a: </div><div>STATIC_ASSERT(MESA_SHADER_TYPES == 3); here? That way in the future when we add tessellation shaders we won't forget to fix it. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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. */ </blockquote><div> </div><div>Style nit-pick: for multi-line comments we put the trailing "*/" on a line by itself. </div><div>With those minor issues addressed, the patch is: Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>