On 03/05/18 22:50, Neil Roberts wrote:
Thanks for the feedback.
I’ve implemented your suggestion as the top two patches on a branch
here:
https://github.com/Igalia/mesa/tree/nroberts/count-uniform-storage
It also fixes a slight bug that it wasn’t passing down the
uniform_storage_locs argument into the recursion.
However I think my preference would be for keeping this as a separate
function because it seems a little confusing to make the function count two
different things depending on a bool argument. Perhaps for the sake of
maintainability it would be good to move it to glsl_types though.
Well we do a similar thing for count_attribute_slots() so I don't think
its to big of a deal.
If there is a strong preference to merge it into one function then of
course I don’t mind.
As long as we document the difference between the two functions having
two would probably be ok also, although my preference is to just have
the one. Up to you others might disagree with me.
Regards,
- Neil
Timothy Arceri <tarc...@itsqueeze.com> writes:
On 18/04/18 00:36, Alejandro Piñeiro wrote:
From: Neil Roberts <nrobe...@igalia.com>
Previously when setting up a uniform it would try to walk the uniform
storage slots and find one that matches the name of the given
variable. However, each variable already has a location which is an
index into the UniformStorage array so we can just directly jump to
the right slot. Some of the variables take up more than one slot so we
still need to calculate how many it uses.
The main reason to do this is to support ARB_gl_spirv because in that
case the uniforms don’t have names so the previous approach won’t
work.
This is a nice improvement away :)
However it might be nicer to just update the existing
glsl_type::uniform_locations() helper
rather than create the custom count_uniform_storage_slots() helper.
e.g
unsigned
glsl_type::uniform_locations(bool uniform_storage_locs) const
{
unsigned size = 0;
switch (this->base_type) {
case GLSL_TYPE_UINT:
case GLSL_TYPE_INT:
case GLSL_TYPE_FLOAT:
case GLSL_TYPE_FLOAT16:
case GLSL_TYPE_DOUBLE:
case GLSL_TYPE_UINT16:
case GLSL_TYPE_UINT8:
case GLSL_TYPE_INT16:
case GLSL_TYPE_INT8:
case GLSL_TYPE_UINT64:
case GLSL_TYPE_INT64:
case GLSL_TYPE_BOOL:
case GLSL_TYPE_SAMPLER:
case GLSL_TYPE_IMAGE:
case GLSL_TYPE_SUBROUTINE:
return 1;
case GLSL_TYPE_STRUCT:
case GLSL_TYPE_INTERFACE:
for (unsigned i = 0; i < this->length; i++)
size += this->fields.structure[i].type->uniform_locations();
return size;
case GLSL_TYPE_ARRAY:
if (!uniform_storage_locs ||
this->fields.array->is_array() ||
this->fields.array->is_interface() ||
this->fields.array->is_record()) {
/* For uniform locations passed to the user via the API we
* count all array elements.
*/
return this->length * this->fields.array->uniform_locations();
} else {
/* For uniform storage the innermost array only uses a
* single slot.
*/
return 1;
}
default:
return 0;
}
}
If you agree this patch is:
Reviewed-by: Timothy Arceri <tarc...@itsqueeze.com>
---
src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 50
+++++++++++++++++++-------
1 file changed, 37 insertions(+), 13 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
index 62b2951432a..cb5e07f74ba 100644
--- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
+++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
@@ -118,35 +118,59 @@ brw_setup_image_uniform_values(gl_shader_stage stage,
}
}
+static unsigned
+count_uniform_storage_slots(const struct glsl_type *type)
+{
+ /* gl_uniform_storage can cope with one level of array, so if the
+ * type is a composite type or an array where each element occupies
+ * more than one slot than we need to recursively process it.
+ */
+ if (glsl_type_is_struct(type)) {
+ unsigned location_count = 0;
+
+ for (unsigned i = 0; i < glsl_get_length(type); i++) {
+ const struct glsl_type *field_type = glsl_get_struct_field(type, i);
+
+ location_count += count_uniform_storage_slots(field_type);
+ }
+
+ return location_count;
+ }
+
+ if (glsl_type_is_array(type)) {
+ const struct glsl_type *element_type = glsl_get_array_element(type);
+
+ if (glsl_type_is_array(element_type) ||
+ glsl_type_is_struct(element_type)) {
+ unsigned element_count = count_uniform_storage_slots(element_type);
+ return element_count * glsl_get_length(type);
+ }
+ }
+
+ return 1;
+}
+
static void
brw_nir_setup_glsl_uniform(gl_shader_stage stage, nir_variable *var,
const struct gl_program *prog,
struct brw_stage_prog_data *stage_prog_data,
bool is_scalar)
{
- int namelen = strlen(var->name);
-
/* The data for our (non-builtin) uniforms is stored in a series of
* gl_uniform_storage structs for each subcomponent that
* glGetUniformLocation() could name. We know it's been set up in the
same
- * order we'd walk the type, so walk the list of storage and find anything
- * with our name, or the prefix of a component that starts with our name.
+ * order we'd walk the type, so walk the list of storage that matches the
+ * range of slots covered by this variable.
*/
unsigned uniform_index = var->data.driver_location / 4;
- for (unsigned u = 0; u < prog->sh.data->NumUniformStorage; u++) {
+ unsigned num_slots = count_uniform_storage_slots(var->type);
+ for (unsigned u = 0; u < num_slots; u++) {
struct gl_uniform_storage *storage =
- &prog->sh.data->UniformStorage[u];
+ &prog->sh.data->UniformStorage[var->data.location + u];
if (storage->builtin || storage->type->is_sampler())
continue;
- if (strncmp(var->name, storage->name, namelen) != 0 ||
- (storage->name[namelen] != 0 &&
- storage->name[namelen] != '.' &&
- storage->name[namelen] != '[')) {
- continue;
- }
-
if (storage->type->is_image()) {
brw_setup_image_uniform_values(stage, stage_prog_data,
uniform_index, storage);
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
