[Mesa-dev] [PATCH 16/25] i965/fs: Obtain atomic counter locations by recursing through the visitor.

[Paul Berry]

On 2 December 2013 11:39, Francisco Jerez <currojerez at riseup.net> wrote:

> Until now atomic counter built-ins were handled in a way that
> prevented the visitor from encountering atomic counter IR variables
> and dereferences directly.  In the new surface lowering code it's
> going to be more convenient to be able to call back into the visitor
> to let it handle the ugly details of atomic counter array
> dereferences, and it will make sharing the rest of the atomic
> intrinsic handling code easier.
>

The commit message should note that this patch fixes the fs part of the
regression to atomic counters introduced by patch 10, by re-introducing the
call to brw_mark_surface_used().


> ---
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 102
> ++++++++++++++++-----------
>  1 file changed, 60 insertions(+), 42 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index b5957c6..d65809f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -102,34 +102,40 @@ fs_visitor::visit(ir_variable *ir)
>          }
>        }
>     } else if (ir->mode == ir_var_uniform) {
> -      int param_index = uniforms;
> -
> -      /* Thanks to the lower_ubo_reference pass, we will see only
> -       * ir_binop_ubo_load expressions and not ir_dereference_variable
> for UBO
> -       * variables, so no need for them to be in variable_ht.
> -       *
> -       * Atomic counters take no uniform storage, no need to do
> -       * anything here.
> -       */
> -      if (ir->is_in_uniform_block() || ir->type->contains_atomic())
> +      if (ir->is_in_uniform_block()) {
> +         /* Thanks to the lower_ubo_reference pass, we will see only
> +          * ir_binop_ubo_load expressions and not ir_dereference_variable
> for UBO
> +          * variables, so no need for them to be in variable_ht.
> +          */
>           return;
>
> -      if (dispatch_width == 16) {
> -        if (!variable_storage(ir)) {
> -           fail("Failed to find uniform '%s' in 16-wide\n", ir->name);
> -        }
> -        return;
> -      }
> +      } else if (ir->type->contains_atomic()) {
> +         reg = new(this->mem_ctx) fs_reg(ir->atomic.offset);
> +
> +         brw_mark_surface_used(stage_prog_data,
> +                               stage_prog_data->binding_table.abo_start +
> +                               ir->atomic.buffer_index);
>
> -      param_size[param_index] = type_size(ir->type);
> -      if (!strncmp(ir->name, "gl_", 3)) {
> -        setup_builtin_uniform_values(ir);
>        } else {
> -        setup_uniform_values(ir);
> -      }
> +         int param_index = uniforms;
>
> -      reg = new(this->mem_ctx) fs_reg(UNIFORM, param_index);
> -      reg->type = brw_type_for_base_type(ir->type);
> +         if (dispatch_width == 16) {
> +            if (!variable_storage(ir)) {
> +               fail("Failed to find uniform '%s' in 16-wide\n", ir->name);
> +            }
> +            return;
> +         }
> +
> +         param_size[param_index] = type_size(ir->type);
> +         if (!strncmp(ir->name, "gl_", 3)) {
> +            setup_builtin_uniform_values(ir);
> +         } else {
> +            setup_uniform_values(ir);
> +         }
> +
> +         reg = new(this->mem_ctx) fs_reg(UNIFORM, param_index);
> +         reg->type = brw_type_for_base_type(ir->type);
> +      }
>
>     } else if (ir->mode == ir_var_system_value) {
>        if (ir->location == SYSTEM_VALUE_SAMPLE_POS) {
> @@ -182,31 +188,43 @@ fs_visitor::visit(ir_dereference_array *ir)
>     src = this->result;
>     src.type = brw_type_for_base_type(ir->type);
>
> -   if (constant_index) {
> -      assert(src.file == UNIFORM || src.file == GRF);
> -      src.reg_offset += constant_index->value.i[0] * element_size;
> -   } else {
> -      /* Variable index array dereference.  We attach the variable index
> -       * component to the reg as a pointer to a register containing the
> -       * offset.  Currently only uniform arrays are supported in this
> patch,
> -       * and that reladdr pointer is resolved by
> -       * move_uniform_array_access_to_pull_constants().  All other array
> types
> -       * are lowered by lower_variable_index_to_cond_assign().
> -       */
> +   if (ir->array->type->contains_atomic()) {
> +      fs_reg tmp(this, glsl_type::uint_type);
> +
>        ir->array_index->accept(this);
>
> -      fs_reg index_reg;
> -      index_reg = fs_reg(this, glsl_type::int_type);
> -      emit(BRW_OPCODE_MUL, index_reg, this->result, fs_reg(element_size));
> +      emit(MUL(tmp, this->result, ATOMIC_COUNTER_SIZE));
> +      emit(ADD(tmp, tmp, src));
> +      this->result = tmp;
> +
>

I believe we need a call to brw_mark_surface_used() here too.

With those issues addressed, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131231/eaab0adb/attachment-0001.html>