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 <ito...@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);
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
