[Mesa-dev] [PATCHv2 10/23] i965/fs: Stop using the LOAD_PAYLOAD instruction in lower_simd_width.

[Francisco Jerez]

Michael Schellenberger Costa <mschellenbergercosta at googlemail.com>
writes:

> Hi Curro,
>
> Am 04.05.2016 um 06:26 schrieb Francisco Jerez:
>> Instead of using the LOAD_PAYLOAD instruction (emitted through the
>> emit_transpose() helper that is no longer useful and this commit
>> removes) which had to be marked force_writemask_all in some cases,
>> emit a series of moves to apply proper channel enable signals to the
>> destination.  Until now lower_simd_width() had mainly been used to
>> lower things that invariably had a basic block-local temporary as
>> destination so it didn't seem like a big deal, but I found it to be
>> the reason for several Piglit regressions in my SIMD32 branch and
>> Igalia discovered the same issue independently while working on FP64
>> support.
>> ---
>> This is taken from the following WIP series:
>>   https://cgit.freedesktop.org/~currojerez/mesa/log/?h=i965-late-simd-lowering
>>
>> See also:
>>   https://lists.freedesktop.org/archives/mesa-dev/2016-May/115596.html
>>
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 58 +++++++++++-------------------------
>>  1 file changed, 18 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 4b6aa67..34599cb 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -4625,29 +4625,6 @@ get_lowered_simd_width(const struct brw_device_info *devinfo,
>>     }
>>  }
>>  
>> -/**
>> - * The \p rows array of registers represents a \p num_rows by \p num_columns
>> - * matrix in row-major order, write it in column-major order into the register
>> - * passed as destination.  \p stride gives the separation between matrix
>> - * elements in the input in fs_builder::dispatch_width() units.
>> - */
>> -static void
>> -emit_transpose(const fs_builder &bld,
>> -               const fs_reg &dst, const fs_reg *rows,
>> -               unsigned num_rows, unsigned num_columns, unsigned stride)
>> -{
>> -   fs_reg *const components = new fs_reg[num_rows * num_columns];
>> -
>> -   for (unsigned i = 0; i < num_columns; ++i) {
>> -      for (unsigned j = 0; j < num_rows; ++j)
>> -         components[num_rows * i + j] = offset(rows[j], bld, stride * i);
>> -   }
>> -
>> -   bld.LOAD_PAYLOAD(dst, components, num_rows * num_columns, 0);
>> -
>> -   delete[] components;
>> -}
>> -
>>  bool
>>  fs_visitor::lower_simd_width()
>>  {
>> @@ -4698,16 +4675,19 @@ fs_visitor::lower_simd_width()
>>                 if (inst->src[j].file != BAD_FILE &&
>>                     !is_uniform(inst->src[j])) {
>>                    /* Get the i-th copy_width-wide chunk of the source. */
>> -                  const fs_reg src = horiz_offset(inst->src[j], copy_width * i);
>> +                  const fs_builder cbld = lbld.group(copy_width, 0);
>> +                  const fs_reg src = offset(inst->src[j], cbld, i);
>>                    const unsigned src_size = inst->components_read(j);
>>  
>> -                  /* Use a trivial transposition to copy one every n
>> -                   * copy_width-wide components of the register into a
>> -                   * temporary passed as source to the lowered instruction.
>> +                  /* Copy one every n copy_width-wide components of the
> That first sentence is really unclear. Shouldnt it sound "Copy each
> copy_width-wide component of..."
>
Nope, it really is copying one every n copy_width chunks of the
register, any suggestions to make the wording more clear?

> Michael
>> +                   * register into a temporary passed as source to the lowered
>> +                   * instruction.
>>                     */
>>                    split_inst.src[j] = lbld.vgrf(inst->src[j].type, src_size);
>> -                  emit_transpose(lbld.group(copy_width, 0),
>> -                                 split_inst.src[j], &src, 1, src_size, n);
>> +
>> +                  for (unsigned k = 0; k < src_size; ++k)
>> +                     cbld.MOV(offset(split_inst.src[j], lbld, k),
>> +                              offset(src, cbld, n * k));
>>                 }
>>              }
>>  
>> @@ -4726,20 +4706,18 @@ fs_visitor::lower_simd_width()
>>           }
>>  
>>           if (inst->regs_written) {
>> -            /* Distance between useful channels in the temporaries, skipping
>> -             * garbage if the lowered instruction is wider than the original.
>> -             */
>> -            const unsigned m = lower_width / copy_width;
>> +            const fs_builder lbld = ibld.group(lower_width, 0);
>>  
>>              /* Interleave the components of the result from the lowered
>> -             * instructions.  We need to set exec_all() when copying more than
>> -             * one half per component, because LOAD_PAYLOAD (in terms of which
>> -             * emit_transpose is implemented) can only use the same channel
>> -             * enable signals for all of its non-header sources.
>> +             * instructions.
>>               */
>> -            emit_transpose(ibld.exec_all(inst->exec_size > copy_width)
>> -                               .group(copy_width, 0),
>> -                           inst->dst, dsts, n, dst_size, m);
>> +            for (unsigned i = 0; i < dst_size; ++i) {
>> +               for (unsigned j = 0; j < n; ++j) {
>> +                  const fs_builder cbld = ibld.group(copy_width, j);
>> +                  cbld.MOV(offset(inst->dst, cbld, n * i + j),
>> +                           offset(dsts[j], lbld, i));
>> +               }
>> +            }
>>           }
>>  
>>           inst->remove(block);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160504/97bd7d0d/attachment.sig>