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. If there is a strong preference to merge it into one function then of course I don’t mind. 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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
