[Mesa-dev] [PATCH v2 03/15] i965/fs_cse: Factor out code to create copy instructions

[Jason Ekstrand]

On Thu, May 7, 2015 at 5:52 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Tue, May 05, 2015 at 06:28:06PM -0700, Jason Ekstrand wrote:
>> v2: Get rid of the block parameter and make src a const reference
>>
>> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 75 ++++++++++++++++----------------
>>  1 file changed, 38 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>> index 43370cb..9c4ed0b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>> @@ -185,6 +185,29 @@ instructions_match(fs_inst *a, fs_inst *b, bool *negate)
>>            operands_match(a, b, negate);
>>  }
>>
>> +static fs_inst *
>> +create_copy_instr(fs_visitor *v, fs_inst *inst, fs_reg src, bool negate)
>
> Did you mean 'src' to be constant reference? It is only used for reading
> so it could be - you claim this in the commit message yourself :)

Oops...  I think what happened is that I tried to do it for
is_copy_payload not create_copy_instr.  But then is_copy_payload does
actually change it so I put it back and somehow my brain leaked it
into the commit message.  Unfortunately, it's already pushed so I
can't change it now.  However, I could make a fixup if you'd like.
--Jason

>> +{
>> +   int written = inst->regs_written;
>> +   int dst_width = inst->dst.width / 8;
>> +   fs_reg dst = inst->dst;
>> +   fs_inst *copy;
>> +
>> +   if (written > dst_width) {
>> +      fs_reg *sources = ralloc_array(v->mem_ctx, fs_reg, written / dst_width);
>> +      for (int i = 0; i < written / dst_width; i++)
>> +         sources[i] = offset(src, i);
>> +      copy = v->LOAD_PAYLOAD(dst, sources, written / dst_width);
>> +   } else {
>> +      copy = v->MOV(dst, src);
>> +      copy->force_writemask_all = inst->force_writemask_all;
>> +      copy->src[0].negate = negate;
>> +   }
>> +   assert(copy->regs_written == written);
>> +
>> +   return copy;
>> +}
>> +
>>  bool
>>  fs_visitor::opt_cse_local(bblock_t *block)
>>  {
>> @@ -230,49 +253,27 @@ fs_visitor::opt_cse_local(bblock_t *block)
>>              bool no_existing_temp = entry->tmp.file == BAD_FILE;
>>              if (no_existing_temp && !entry->generator->dst.is_null()) {
>>                 int written = entry->generator->regs_written;
>> -               int dst_width = entry->generator->dst.width / 8;
>> -               assert(written % dst_width == 0);
>> -
>> -               fs_reg orig_dst = entry->generator->dst;
>> -               fs_reg tmp = fs_reg(GRF, alloc.allocate(written),
>> -                                   orig_dst.type, orig_dst.width);
>> -               entry->tmp = tmp;
>> -               entry->generator->dst = tmp;
>> -
>> -               fs_inst *copy;
>> -               if (written > dst_width) {
>> -                  fs_reg *sources = ralloc_array(mem_ctx, fs_reg, written / dst_width);
>> -                  for (int i = 0; i < written / dst_width; i++)
>> -                     sources[i] = offset(tmp, i);
>> -                  copy = LOAD_PAYLOAD(orig_dst, sources, written / dst_width);
>> -               } else {
>> -                  copy = MOV(orig_dst, tmp);
>> -                  copy->force_writemask_all =
>> -                     entry->generator->force_writemask_all;
>> -               }
>> +               assert((written * 8) % entry->generator->dst.width == 0);
>> +
>> +               entry->tmp = fs_reg(GRF, alloc.allocate(written),
>> +                                   entry->generator->dst.type,
>> +                                   entry->generator->dst.width);
>> +
>> +               fs_inst *copy = create_copy_instr(this, entry->generator,
>> +                                                 entry->tmp, false);
>>                 entry->generator->insert_after(block, copy);
>> +
>> +               entry->generator->dst = entry->tmp;
>>              }
>>
>>              /* dest <- temp */
>>              if (!inst->dst.is_null()) {
>> -               int written = inst->regs_written;
>> -               int dst_width = inst->dst.width / 8;
>> -               assert(written == entry->generator->regs_written);
>> -               assert(dst_width == entry->generator->dst.width / 8);
>> +               assert(inst->regs_written == entry->generator->regs_written);
>> +               assert(inst->dst.width == entry->generator->dst.width);
>>                 assert(inst->dst.type == entry->tmp.type);
>> -               fs_reg dst = inst->dst;
>> -               fs_reg tmp = entry->tmp;
>> -               fs_inst *copy;
>> -               if (written > dst_width) {
>> -                  fs_reg *sources = ralloc_array(mem_ctx, fs_reg, written / dst_width);
>> -                  for (int i = 0; i < written / dst_width; i++)
>> -                     sources[i] = offset(tmp, i);
>> -                  copy = LOAD_PAYLOAD(dst, sources, written / dst_width);
>> -               } else {
>> -                  copy = MOV(dst, tmp);
>> -                  copy->force_writemask_all = inst->force_writemask_all;
>> -                  copy->src[0].negate = negate;
>> -               }
>> +
>> +               fs_inst *copy = create_copy_instr(this, inst,
>> +                                                 entry->tmp, negate);
>>                 inst->insert_before(block, copy);
>>              }
>>
>> --
>> 2.3.6
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev