[Mesa-dev] [PATCH v3 06/12] i965/fs: Get rid of load/store_foo_indirect

[Kenneth Graunke]

On Tuesday, December 08, 2015 01:46:23 PM Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h       |   2 +-
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 106 ++++++++++++++-----------------
>  src/mesa/drivers/dri/i965/brw_nir.c      |  45 ++++++++++---
>  3 files changed, 84 insertions(+), 69 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index bca4589..b55589f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -278,7 +278,7 @@ public:
>                         unsigned stream_id);
>     void emit_gs_thread_end();
>     void emit_gs_input_load(const fs_reg &dst, const nir_src &vertex_src,
> -                           const fs_reg &indirect_offset, unsigned imm_offset,
> +                           unsigned base_offset, const nir_src &offset_src,
>                             unsigned num_components);
>     void emit_cs_terminate();
>     fs_reg *emit_cs_local_invocation_id_setup();
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 3754622..18c9d65 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1603,29 +1603,31 @@ fs_visitor::emit_gs_vertex(const nir_src &vertex_count_nir_src,
>  void
>  fs_visitor::emit_gs_input_load(const fs_reg &dst,
>                                 const nir_src &vertex_src,
> -                               const fs_reg &indirect_offset,
> -                               unsigned imm_offset,
> +                               unsigned base_offset,
> +                               const nir_src &offset_src,
>                                 unsigned num_components)
>  {
>     struct brw_gs_prog_data *gs_prog_data = (struct brw_gs_prog_data *) prog_data;
>  
> +   nir_const_value *vertex_const = nir_src_as_const_value(vertex_src);
> +   nir_const_value *offset_const = nir_src_as_const_value(offset_src);
> +   const unsigned push_reg_count = gs_prog_data->base.urb_read_length * 8;
> +
>     /* Offset 0 is the VUE header, which contains VARYING_SLOT_LAYER [.y],
>      * VARYING_SLOT_VIEWPORT [.z], and VARYING_SLOT_PSIZ [.w].  Only
>      * gl_PointSize is available as a GS input, however, so it must be that.
>      */
> -   const bool is_point_size =
> -      indirect_offset.file == BAD_FILE && imm_offset == 0;
> -
> -   nir_const_value *vertex_const = nir_src_as_const_value(vertex_src);
> -   const unsigned push_reg_count = gs_prog_data->base.urb_read_length * 8;
> +   const bool is_point_size = (base_offset == 0);
>  
> -   if (indirect_offset.file == BAD_FILE && vertex_const != NULL &&
> -       4 * imm_offset < push_reg_count) {
> -      imm_offset = 4 * imm_offset + vertex_const->u[0] * push_reg_count;
> +   if (offset_const != NULL && vertex_const != NULL &&
> +       4 * offset_const->u[0] < push_reg_count) {

This needs to be

    4 * (base_offset + offset_const->u[0]) < push_reg_count

otherwise we use the push model for inputs that aren't actually pushed,
which breaks around 1500 piglit tests with INTEL_SCALAR_GS=1.

> +      int imm_offset = (base_offset + offset_const->u[0]) * 4 +
> +                       vertex_const->u[0] * push_reg_count;
>        /* This input was pushed into registers. */
>        if (is_point_size) {
>           /* gl_PointSize comes in .w */
> -         bld.MOV(dst, fs_reg(ATTR, imm_offset + 3, dst.type));
> +         assert(imm_offset == 0);
> +         bld.MOV(dst, fs_reg(ATTR, 3, dst.type));

This is wrong.  imm_offset contains the offset derived from the vertex
index, and gl_PointSize exists for multiple vertices.  So it won't be 0.

Please just remove this change, putting it back to:

         bld.MOV(dst, fs_reg(ATTR, imm_offset + 3, dst.type));

That fixes assertion failures in 3 Piglit tests.

With that fixed, patches 1-8 and 11-12 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151208/aa5eaad6/attachment-0001.sig>