[Mesa-dev] [PATCH 16/78] i965/nir/vec4: Implement store_output intrinsic

[Jason Ekstrand]

On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> The index into the output_reg array where to store the destination register is
> fetched from the nir_outputs map built during nir_setup_outputs stage.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> index 8a2d335..55d4490 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> @@ -520,10 +520,23 @@ vec4_visitor::nir_emit_intrinsic(nir_intrinsic_instr *instr)
>     }
>
>     case nir_intrinsic_store_output_indirect:
> +      has_indirect = true;
>        /* fallthrough */
> -   case nir_intrinsic_store_output:
> -      /* @TODO: Not yet implemented */
> +   case nir_intrinsic_store_output: {
> +      int offset = instr->const_index[0];
> +      int output = nir_outputs[offset];
> +
> +      src = get_nir_src(instr->src[0], nir_output_types[offset]);
> +      dest = dst_reg(src);
> +
> +      dest.writemask = brw_writemask_for_size(instr->num_components);
> +
> +      if (has_indirect)
> +         dest.reladdr = new(mem_ctx) src_reg(get_nir_src(instr->src[1]));
> +
> +      output_reg[output] = dest;

I'm very confused about the amount of indirection going on here.  It
seems to me that we should be setting these outputs up in
setup_outputs() rather than storring off a map from ints to other ints
and setting it up here.  I didn't make this comment on the patch for
setup_outputs() because I wanted to wait to see it used before I
commented on it.

I'm guessing you did it this way because the nir_assign_var_locations
is giving you bogus values.  If so, then it might be better to just
assign variable locations in setup_outputs() rather than having a
remap table.  The whole point of nir_lower_io is to make IO easy for
the back-end.  If you need a re-map table, then it's no longer making
it easy and we need to think more about what's going on.
--Jason

>        break;
> +   }
>
>     case nir_intrinsic_load_vertex_id:
>        unreachable("should be lowered by lower_vertex_id()");
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev