[Mesa-dev] [PATCH 10/11] i965/nir: Pull GLSL uniform handling into a common function

Iago Toral itoral at igalia.com
Fri Oct 2 04:14:09 PDT 2015


On Wed, 2015-09-30 at 18:41 -0700, Jason Ekstrand wrote:
> The way we deal with GLSL uniforms and builtins is basically the same in
> both the vec4 and the fs backend.  This commit takes the best parts of both
> implementations and pulls the common code into a shared helper function.
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h             |   2 -
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp       |  79 +---------------
>  src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp | 120 +++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_vec4.h           |   2 -
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp     |  74 +--------------
>  5 files changed, 126 insertions(+), 151 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 6231652..0bc639d 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -241,8 +241,6 @@ public:
>     void nir_setup_inputs(nir_shader *shader);
>     void nir_setup_outputs(nir_shader *shader);
>     void nir_setup_uniforms(nir_shader *shader);
> -   void nir_setup_uniform(nir_variable *var);
> -   void nir_setup_builtin_uniform(nir_variable *var);
>     void nir_emit_system_values(nir_shader *shader);
>     void nir_emit_impl(nir_function_impl *impl);
>     void nir_emit_cf_list(exec_list *list);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 3ba6a67..829c663 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -183,15 +183,14 @@ fs_visitor::nir_setup_uniforms(nir_shader *shader)
>     uniforms = shader->num_uniforms;
>  
>     if (shader_prog) {
> +      brw_nir_setup_glsl_uniforms(shader, shader_prog, prog,
> +                                  stage_prog_data, true);
> +
>        foreach_list_typed(nir_variable, var, node, &shader->uniforms) {
>           /* UBO's and atomics don't take up space in the uniform file */
>           if (var->interface_type != NULL || var->type->contains_atomic())
>              continue;
>  
> -         if (strncmp(var->name, "gl_", 3) == 0)
> -            nir_setup_builtin_uniform(var);
> -         else
> -            nir_setup_uniform(var);
>           if(type_size_scalar(var->type) > 0)
>              param_size[var->data.driver_location] = type_size_scalar(var->type);
>        }
> @@ -203,78 +202,6 @@ fs_visitor::nir_setup_uniforms(nir_shader *shader)
>     }
>  }
>  
> -void
> -fs_visitor::nir_setup_uniform(nir_variable *var)
> -{
> -   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 index = var->data.driver_location;
> -   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;
> -      }
> -
> -      if (storage->type->is_image()) {
> -         brw_setup_image_uniform_values(stage, stage_prog_data,
> -                                        index, storage);
> -      } else {
> -         unsigned slots = storage->type->component_slots();
> -         if (storage->array_elements)
> -            slots *= storage->array_elements;
> -
> -         for (unsigned i = 0; i < slots; i++) {
> -            stage_prog_data->param[index++] = &storage->storage[i];
> -         }
> -      }
> -   }
> -}
> -
> -void
> -fs_visitor::nir_setup_builtin_uniform(nir_variable *var)
> -{
> -   const nir_state_slot *const slots = var->state_slots;
> -   assert(var->state_slots != NULL);
> -
> -   unsigned uniform_index = var->data.driver_location;
> -   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.
> -       */
> -      int index = _mesa_add_state_reference(this->prog->Parameters,
> -                                            (gl_state_index *)slots[i].tokens);
> -
> -      /* Add each of the unique swizzles of the element as a parameter.
> -       * This'll end up matching the expected layout of the
> -       * array/matrix/structure we're trying to fill in.
> -       */
> -      int last_swiz = -1;
> -      for (unsigned int j = 0; j < 4; j++) {
> -         int swiz = GET_SWZ(slots[i].swizzle, j);
> -         if (swiz == last_swiz)
> -            break;
> -         last_swiz = swiz;
> -
> -         stage_prog_data->param[uniform_index++] =
> -            &prog->Parameters->ParameterValues[index][swiz];
> -      }
> -   }
> -}
> -
>  static bool
>  emit_system_values_block(nir_block *block, void *void_visitor)
>  {
> diff --git a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> index ede4e91..087a099 100644
> --- a/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_nir_uniforms.cpp
> @@ -23,6 +23,126 @@
>  
>  #include "brw_shader.h"
>  #include "brw_nir.h"
> +#include "glsl/ir.h"
> +#include "glsl/ir_uniform.h"
> +
> +void
> +brw_nir_setup_glsl_builtin_uniform(nir_variable *var,
> +                                   const struct gl_program *prog,
> +                                   struct brw_stage_prog_data *stage_prog_data,
> +                                   unsigned comps_per_unit)
> +

Shouldn't this be static?

> {
> +   const nir_state_slot *const slots = var->state_slots;
> +   assert(var->state_slots != NULL);
> +
> +   unsigned uniform_index = var->data.driver_location * comps_per_unit;
> +   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.
> +       */
> +      int index = _mesa_add_state_reference(prog->Parameters,
> +					    (gl_state_index *)slots[i].tokens);
> +
> +      /* Add each of the unique swizzles of the element as a parameter.
> +       * This'll end up matching the expected layout of the
> +       * array/matrix/structure we're trying to fill in.
> +       */
> +      int last_swiz = -1;
> +      for (unsigned j = 0; j < 4; j++) {
> +         int swiz = GET_SWZ(slots[i].swizzle, j);
> +
> +         /* If we hit a pair of identical swizzles, this means we've hit the
> +          * end of the builtin variable.  In scalar mode, we should just quit
> +          * and move on to the next one.  In vec4, we need to continue and pad
> +          * it out to 4 components.
> +          */
> +         if (swiz == last_swiz && j >= comps_per_unit)
> +            break;

Not that it matters much, but why not just:

if (swiz == last_swiz && comps_per_unit == 1)
   break;

If we only want to break in the scalar case I think that makes it more
obvious.

> +         last_swiz = swiz;
> +
> +         stage_prog_data->param[uniform_index++] =
> +            &prog->Parameters->ParameterValues[index][swiz];
> +      }
> +   }
> +}
> +
> +void
> +brw_nir_setup_glsl_uniform(gl_shader_stage stage, nir_variable *var,
> +                           struct gl_shader_program *shader_prog,
> +                           struct brw_stage_prog_data *stage_prog_data,
> +                           unsigned comps_per_unit)

Shouldn't this be static?

> +{
> +   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 index = var->data.driver_location * comps_per_unit;
> +   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;
> +      }
> +
> +      if (storage->type->is_image()) {
> +         brw_setup_image_uniform_values(stage, stage_prog_data, index, storage);
> +      } else {
> +         gl_constant_value *components = storage->storage;
> +         unsigned vector_count = (MAX2(storage->array_elements, 1) *
> +                                  storage->type->matrix_columns);
> +         unsigned vector_size = storage->type->vector_elements;
> +
> +         for (unsigned s = 0; s < vector_count; s++) {
> +            unsigned i;
> +            for (i = 0; i < vector_size; i++) {
> +               stage_prog_data->param[index++] = components++;
> +            }
> +
> +            /* Pad out with zeros if needed (only needed for vec4) */
> +            for (; i < comps_per_unit; i++) {
> +               static const gl_constant_value zero = { 0.0 };
> +               stage_prog_data->param[index++] = &zero;
> +            }
> +         }
> +      }
> +   }
> +}
> +
> +void
> +brw_nir_setup_glsl_uniforms(nir_shader *shader,
> +                            struct gl_shader_program *shader_prog,
> +                            const struct gl_program *prog,
> +                            struct brw_stage_prog_data *stage_prog_data,
> +                            bool is_scalar)

As I mentioned in a previous patch, you need to pull the header
declaration of this function into this patch.

With that and the static functions, this is:

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

> +{
> +   unsigned comps_per_unit = is_scalar ? 1 : 4;
> +
> +   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())
> +         continue;
> +
> +      if (strncmp(var->name, "gl_", 3) == 0) {
> +         brw_nir_setup_glsl_builtin_uniform(var, prog, stage_prog_data,
> +                                            comps_per_unit);
> +      } else {
> +         brw_nir_setup_glsl_uniform(shader->stage, var, shader_prog,
> +                                    stage_prog_data, comps_per_unit);
> +      }
> +   }
> +}
>  
>  void
>  brw_nir_setup_arb_uniforms(nir_shader *shader, struct gl_program *prog,
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> index b3a62d2..098fff01 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -334,8 +334,6 @@ public:
>     virtual void emit_nir_code();
>     virtual void nir_setup_inputs(nir_shader *shader);
>     virtual void nir_setup_uniforms(nir_shader *shader);
> -   virtual void nir_setup_uniform(nir_variable *var);
> -   virtual void nir_setup_builtin_uniform(nir_variable *var);
>     virtual void nir_setup_system_value_intrinsic(nir_intrinsic_instr *instr);
>     virtual void nir_setup_system_values(nir_shader *shader);
>     virtual void nir_emit_impl(nir_function_impl *impl);
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> index 99fd71f..36bb35f 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> @@ -137,6 +137,9 @@ vec4_visitor::nir_setup_uniforms(nir_shader *shader)
>     uniforms = shader->num_uniforms;
>  
>     if (shader_prog) {
> +      brw_nir_setup_glsl_uniforms(shader, shader_prog, prog,
> +                                  stage_prog_data, false);
> +
>        foreach_list_typed(nir_variable, var, node, &shader->uniforms) {
>           /* UBO's, atomics and samplers don't take up space in the
>              uniform file */
> @@ -146,11 +149,6 @@ vec4_visitor::nir_setup_uniforms(nir_shader *shader)
>           }
>  
>           uniform_size[var->data.driver_location] = type_size_vec4(var->type);
> -
> -         if (strncmp(var->name, "gl_", 3) == 0)
> -            nir_setup_builtin_uniform(var);
> -         else
> -            nir_setup_uniform(var);
>        }
>     } else {
>        brw_nir_setup_arb_uniforms(shader, prog, stage_prog_data);
> @@ -161,72 +159,6 @@ vec4_visitor::nir_setup_uniforms(nir_shader *shader)
>  }
>  
>  void
> -vec4_visitor::nir_setup_uniform(nir_variable *var)
> -{
> -   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 index = var->data.driver_location * 4;
> -    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);
> -
> -       for (unsigned s = 0; s < vector_count; s++) {
> -          int i;
> -          for (i = 0; i < storage->type->vector_elements; i++) {
> -             stage_prog_data->param[index++] = components++;
> -          }
> -          for (; i < 4; i++) {
> -             static const gl_constant_value zero = { 0.0 };
> -             stage_prog_data->param[index++] = &zero;
> -          }
> -       }
> -    }
> -}
> -
> -void
> -vec4_visitor::nir_setup_builtin_uniform(nir_variable *var)
> -{
> -   const nir_state_slot *const slots = var->state_slots;
> -   assert(var->state_slots != NULL);
> -
> -   unsigned uniform_index = var->data.driver_location * 4;
> -   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(prog->Parameters,
> -					    (gl_state_index *)slots[i].tokens);
> -      gl_constant_value *values =
> -         &prog->Parameters->ParameterValues[index][0];
> -
> -      for (unsigned j = 0; j < 4; j++)
> -         stage_prog_data->param[uniform_index++] =
> -            &values[GET_SWZ(slots[i].swizzle, j)];
> -   }
> -}
> -
> -void
>  vec4_visitor::nir_emit_impl(nir_function_impl *impl)
>  {
>     nir_locals = ralloc_array(mem_ctx, dst_reg, impl->reg_alloc);




More information about the mesa-dev mailing list