Hi Jason,
On Mon, 2015-06-29 at 16:22 -0700, Jason Ekstrand wrote:
> On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <el...@igalia.com> wrote:
> > From: Iago Toral Quiroga <ito...@igalia.com>
> >
> > This is based on similar code existing in vec4_visitor. It builds the
> > uniform register file iterating through each uniform variable. It
> > also stores the index of each register at the corresponding offset
> > in a map. This map will later be used by load_uniform intrinsic
> > instructions to build the correct UNIFORM source register.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580
> > ---
> > src/mesa/drivers/dri/i965/brw_vec4.h | 2 +
> > src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 115
> > ++++++++++++++++++++++++++++-
> > 2 files changed, 114 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h
> > b/src/mesa/drivers/dri/i965/brw_vec4.h
> > index 673df4e..6535f19 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> > @@ -414,6 +414,8 @@ public:
> > src_reg *nir_inputs;
> > int *nir_outputs;
> > brw_reg_type *nir_output_types;
> > + unsigned *nir_uniform_offset;
> > + unsigned *nir_uniform_driver_location;
> >
> > protected:
> > void emit_vertex();
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > index 2d457a6..40ec66f 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > @@ -106,19 +106,128 @@ vec4_visitor::nir_setup_outputs(nir_shader *shader)
> > void
> > vec4_visitor::nir_setup_uniforms(nir_shader *shader)
> > {
> > - /* @TODO: Not yet implemented */
> > + uniforms = 0;
> > +
> > + nir_uniform_offset =
> > + rzalloc_array(mem_ctx, unsigned, this->uniform_array_size);
> > + memset(nir_uniform_offset, 0, this->uniform_array_size *
> > sizeof(unsigned));
>
> rzalloc memsets the whole thing to 0 for you, this memset is redundant.
>
> > +
> > + nir_uniform_driver_location =
> > + rzalloc_array(mem_ctx, unsigned, this->uniform_array_size);
> > + memset(nir_uniform_driver_location, 0,
> > + this->uniform_array_size * sizeof(unsigned));
>
> Same here.
Oh, right.
> > +
> > + if (shader_prog) {
> > + foreach_list_typed(nir_variable, var, node, &shader->uniforms) {
> > + /* UBO's, atomics and samplers don't take up space in the
> > + uniform file */
> > + if (var->interface_type != NULL || var->type->contains_atomic() ||
> > + type_size(var->type) == 0) {
>
> I'm curious as to why you have this extra type_size() == 0 condition.
> We don't have that in the FS NIR code. What caused you to add it?
Take this piglit test for example:
bin/textureSize vs isampler1D -auto -fbo
here, 'tex' is a uniform of size 0 since type_size() returns 0 for all
sampler types. If we do not ignore these, we will try to store uniform
information for them in the various structures we have to track uniform
data, like uniform_size[] and others. The size allocated for these
arrays is computed by in the vec4_visitor constructor based on
stage_prog_data->nr_params (uniform_array_size) and that does not seem
to make room for zero-sized uniforms. Without that check we would
process more uniforms than uniform_array_size and overflow the arrays we
allocate to track uniform information. I understand that
stage_prog_data->nr_params does not track uniforms that don't use
register space, so skipping uniforms with no size seems to make sense
here.
Notice that this is done in the current vec4_visitor too, when we visit
the variable in vec4_visitor::visit(ir_variable *ir), for ir_var_uniform
there is this code:
if (ir->is_in_uniform_block() || type_size(ir->type) == 0)
return;
> > + continue;
> > + }
> > +
> > + assert(uniforms < uniform_array_size);
> > + this->uniform_size[uniforms] = type_size(var->type);
> > +
> > + if (strncmp(var->name, "gl_", 3) == 0)
> > + nir_setup_builtin_uniform(var);
> > + else
> > + nir_setup_uniform(var);
> > + }
> > + } else {
> > + /* ARB_vertex_program is not supported yet */
> > + assert("Not implemented");
> > + }
> > }
> >
> > void
> > vec4_visitor::nir_setup_uniform(nir_variable *var)
> > {
> > - /* @TODO: Not yet implemented */
> > + int namelen = strlen(var->name);
> > +
> > + /* The data for our (non-builtin) uniforms is stored in a series of
> > + * gl_uniform_driver_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.
> > + */
> > + unsigned offset = 0;
> > + for (unsigned u = 0; u < shader_prog->NumUniformStorage; u++) {
> > + struct gl_uniform_storage *storage =
> > &shader_prog->UniformStorage[u];
> > +
> > + if (storage->builtin)
> > + continue;
> > +
> > + if (strncmp(var->name, storage->name, namelen) != 0 ||
> > + (storage->name[namelen] != 0 &&
> > + storage->name[namelen] != '.' &&
> > + storage->name[namelen] != '[')) {
> > + continue;
> > + }
> > +
> > + gl_constant_value *components = storage->storage;
> > + unsigned vector_count = (MAX2(storage->array_elements, 1) *
> > + storage->type->matrix_columns);
>
> In the FS backend, we simply use storage->type->component_slots().
> Why can't we do that here? It seems to be performing the same
> calculation.
This does not seem to do what we want in the vec4 backend. For example,
consider the following piglit test:
tests/spec/gl-3.1/attributeless-vertexid.shader_test
In that test we have something like this:
vec2[](
vec2(-1, 1),
vec2(-1,-1),
vec2( 1,-1),
vec2( 1, 1)
);
And for that I see:
storage->type->component_slots() = 2
This is because storage->type is glsl_type::_vec2_type. This is not
convenient for us in the vec4 backend because it is giving a scalar
count and we want to compute the count in units of vec4.
> > +
> > + for (unsigned s = 0; s < vector_count; s++) {
> > + assert(uniforms < uniform_array_size);
> > + uniform_vector_size[uniforms] = storage->type->vector_elements;
> > +
> > + int i;
> > + for (i = 0; i < uniform_vector_size[uniforms]; i++) {
> > + stage_prog_data->param[uniforms * 4 + i] = components;
> > + components++;
> > + }
> > + for (; i < 4; i++) {
> > + static gl_constant_value zero = { 0.0 };
>
> This should probably be const.
Yes, I will fix this.
> > + stage_prog_data->param[uniforms * 4 + i] = &zero;
> > + }
> > +
> > + int uniform_offset = var->data.driver_location + offset;
> > + nir_uniform_offset[uniform_offset] = uniforms;
> > + offset++;
> > +
> > + nir_uniform_driver_location[uniforms] =
> > var->data.driver_location;
> > + uniforms++;
> > + }
> > + }
> > }
> >
> > void
> > vec4_visitor::nir_setup_builtin_uniform(nir_variable *var)
> > {
> > - /* @TODO: Not yet implemented */
> > + const nir_state_slot *const slots = var->state_slots;
> > + assert(var->state_slots != NULL);
> > +
> > + unsigned offset = 0;
> > + for (unsigned int i = 0; i < var->num_state_slots; i++) {
> > + /* This state reference has already been setup by ir_to_mesa,
> > + * but we'll get the same index back here. We can reference
> > + * ParameterValues directly, since unlike brw_fs.cpp, we never
> > + * add new state references during compile.
> > + */
> > + int index = _mesa_add_state_reference(this->prog->Parameters,
> > + (gl_state_index
> > *)slots[i].tokens);
> > + gl_constant_value *values =
> > + &this->prog->Parameters->ParameterValues[index][0];
> > +
> > + assert(this->uniforms < uniform_array_size);
> > +
> > + for (unsigned j = 0; j < 4; j++)
> > + stage_prog_data->param[this->uniforms * 4 + j] =
> > + &values[GET_SWZ(slots[i].swizzle, j)];
> > +
> > + this->uniform_vector_size[this->uniforms] =
> > + (var->type->is_scalar() || var->type->is_vector() ||
> > + var->type->is_matrix() ? var->type->vector_elements : 4);
> > +
> > + int uniform_offset = var->data.driver_location + offset;
> > + nir_uniform_offset[uniform_offset] = uniforms;
> > + offset++;
> > +
> > + nir_uniform_driver_location[uniforms] = var->data.driver_location;
> > + this->uniforms++;
> > + }
> > }
> >
> > void
> > --
> > 2.1.4
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
