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

[Jason Ekstrand]

On Tue, Dec 8, 2015 at 4:44 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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.

Fixed both locally.  Thanks for the careful review and for watching
out for scalar GS!

> With that fixed, patches 1-8 and 11-12 are:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks!
--Jason