On 05/19/2014 07:51 PM, Ian Romanick wrote:
On 04/09/2014 02:56 AM, Tapani Pälli wrote:
Patch initializes the UniformRemapTable for explicit locations. This
needs to happen before optimizations to make sure all inactive uniforms
get their explicit locations correctly.
Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
---
src/glsl/linker.cpp | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 7c194a2..1b4cb63 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -74,6 +74,7 @@
#include "link_varyings.h"
#include "ir_optimization.h"
#include "ir_rvalue_visitor.h"
+#include "ir_uniform.h"
extern "C" {
#include "main/shaderobj.h"
@@ -2089,6 +2090,100 @@ check_image_resources(struct gl_context *ctx, struct
gl_shader_program *prog)
linker_error(prog, "Too many combined image uniforms and fragment
outputs");
}
+
+/**
+ * Initializes explicit location slots point to -1 for a variable,
+ * checks for overlaps between other uniforms using explicit locations.
+ */
+static bool
+reserve_explicit_locations(struct gl_shader_program *prog,
+ string_to_uint_map *map, ir_variable *var)
+{
+ unsigned max_loc = var->data.location + var->type->component_slots() - 1;
+
+ /* Resize remap table if locations do not fit in the current one. */
+ if (max_loc + 1 > prog->NumUniformRemapTable) {
+ prog->UniformRemapTable =
+ reralloc(prog, prog->UniformRemapTable,
+ gl_uniform_storage *,
+ max_loc + 1);
+ prog->NumUniformRemapTable = max_loc + 1;
+ }
+
+ for (unsigned i = 0; i < var->type->component_slots(); i++) {
You should check the code that gets generated for this. I suspect this
will translate to a call to component_slots per iteration of the loop.
Maybe just call it once above (since it is also used to calculate max_loc).
OK, will change.
+ unsigned loc = var->data.location + i;
+
+ /* Check if location is already used. */
+ if (prog->UniformRemapTable[loc] == (gl_uniform_storage *) -1) {
So... -1 means that an inactive uniform has that location explicitly
assigned? I'm inferring that from comments in the next patch. Maybe we
should have a descriptive #define
#define INACTIVE_UNIFORM_EXPLICIT_LOCATION ((gl_uniform_storage *) -1)
Yep, makes it more easier to read.
+
+ /* Possibly same uniform from a different stage, this is ok. */
+ unsigned hash_loc;
+ if (map->get(hash_loc, var->name) && hash_loc == loc - i)
+ continue;
+
+ /* ARB_explicit_uniform_location specification states:
+ *
+ * "No two default-block uniform variables in the program can have
+ * the same location, even if they are unused, otherwise a
compiler
+ * or linker error will be generated."
+ */
+ linker_error(prog, "location qualifier "
+ "for uniform %s "
+ "overlaps previously used location",
+ var->name);
Minor nit (which you can take or leave). I usually like to have fewer
breaks in strings. I would have split this up as:
linker_error(prog,
"location qualifier for uniform %s overlaps "
"previously used location",
var->name);
ok
+ return false;
+ }
+
+ prog->UniformRemapTable[loc] = (gl_uniform_storage *) -1;
+ }
+
+ /* Note, base location used for arrays. */
+ map->put(var->data.location, var->name);
+
+ return true;
+}
+
+/**
+ * Check and reserve all explicit uniform locations, called before
+ * any optimizations happen to handle also inactive uniforms and
+ * inactive array elements that may get trimmed away.
+ */
+static void
+check_explicit_uniform_locations(struct gl_context *ctx,
+ struct gl_shader_program *prog)
+{
+ if (!ctx->Extensions.ARB_explicit_uniform_location)
+ return;
+
+ /* This map is used to detect if overlapping explicit locations
+ * occur with the same uniform (from different stage) or a different one.
+ */
+ string_to_uint_map *uniform_map = new string_to_uint_map;
+
+ for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
+ struct gl_shader *sh = prog->_LinkedShaders[i];
+
+ if (!sh)
+ continue;
+
+ foreach_list(node, sh->ir) {
+ ir_variable *var = ((ir_instruction *)node)->as_variable();
+ if ((var && var->data.mode == ir_var_uniform) &&
+ var->data.explicit_location) {
+ if (!reserve_explicit_locations(prog, uniform_map, var))
+ return;
+
+ /* Initialize locations that were allocated but left unused. */
+ for (unsigned i = 0; i < prog->NumUniformRemapTable; i++)
+ if (prog->UniformRemapTable[i] != (gl_uniform_storage *) -1)
+ prog->UniformRemapTable[i] = NULL;
I'm confused by this... what's in UniformRemapTable on entry to this
function? Random garbage (this might happen to be ~0)?
You are right, this needs fixing. I will move initialization of unused
slots in where allocation is made.
+ }
+ }
+ }
+
+ delete uniform_map;
+}
+
void
link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
{
@@ -2232,6 +2327,10 @@ link_shaders(struct gl_context *ctx, struct
gl_shader_program *prog)
break;
}
+ check_explicit_uniform_locations(ctx, prog);
+ if (!prog->LinkStatus)
+ goto done;
+
/* Validate the inputs of each stage with the output of the preceding
* stage.
*/
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
