[Mesa-dev] [PATCH] i965/fs: Add pass to rename registers to break live ranges.

[Matt Turner]

On Wed, Aug 13, 2014 at 8:36 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> On Wed, Aug 13, 2014 at 1:22 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> From: Kenneth Graunke <kenneth at whitecape.org>
>>
>> The pass breaks live ranges of virtual registers by allocating new
>> registers when it sees an assignment to a virtual GRF it's already seen
>> written.
>>
>> total instructions in shared programs: 4337879 -> 4335014 (-0.07%)
>> instructions in affected programs:     343865 -> 341000 (-0.83%)
>> GAINED:                                46
>> LOST:                                  1
>>
>> Reviewed-by: Eric Anholt <eric at anholt.net>
>> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 67 ++++++++++++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_fs.h   |  1 +
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index cc5ec16..6ec1c8f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -2068,6 +2068,72 @@ fs_visitor::opt_algebraic()
>>  }
>>
>>  bool
>> +fs_visitor::opt_register_renaming()
>> +{
>> +   bool progress = false;
>> +   int depth = 0;
>> +
>> +   int remap[virtual_grf_count];
>> +   memset(remap, -1, sizeof(int) * virtual_grf_count);
>> +
>> +   foreach_in_list(fs_inst, inst, &this->instructions) {
>> +      if (inst->opcode == BRW_OPCODE_IF || inst->opcode == BRW_OPCODE_DO) {
>> +         depth++;
>> +      } else if (inst->opcode == BRW_OPCODE_ENDIF ||
>> +                 inst->opcode == BRW_OPCODE_WHILE) {
>> +         depth--;
>> +      }
>> +
>> +      /* Rewrite instruction sources. */
>> +      for (int i = 0; i < inst->sources; i++) {
>> +         if (inst->src[i].file == GRF &&
>> +             remap[inst->src[i].reg] != -1 &&
>> +             remap[inst->src[i].reg] != inst->src[i].reg) {
>> +            inst->src[i].reg = remap[inst->src[i].reg];
>> +            progress = true;
>> +         }
>> +      }
>> +
>> +      int dst = inst->dst.reg;
>> +
>> +      if (depth == 0 &&
>> +          inst->dst.file == GRF &&
>> +          virtual_grf_sizes[inst->dst.reg] == 1 &&
>
> We can just use dst here instead of inst->dst.reg

Yeah, a bit weird to use inst->dst.file in one conditional, and not
use inst->dst.reg in the next though.

>> +          !inst->is_partial_write()) {
>> +         if (remap[dst] == -1) {
>> +            remap[dst] = dst;
>> +         } else {
>> +            remap[dst] = virtual_grf_alloc(virtual_grf_sizes[dst]);
>
> Since this code only gets run when virtual_grf_sizes[dst] == 1, we can
> just do virtual_grf_alloc(1).
>
> Also, I think you're forgetting to set the type of the new destination
> (i.e. remap[dst]).
>
> Finally, I don't think this should matter in practice, but if the
> instruction's destination is a hardware register we should probably
> bail out.

HW_REG's register file and number are stored in the fixed_hw_reg
struct, not in the regular file/reg fields.

>> +            inst->dst.reg = remap[dst];
>> +            progress = true;
>> +         }
>> +      } else if (inst->dst.file == GRF &&
>> +                 remap[dst] != -1 &&
>> +                 remap[dst] != dst) {
>> +         inst->dst.reg = remap[dst];
>
> I think you're forgetting to set the type here too.

remap[] is just an array of the register numbers, so we're not
overwriting the type of inst->dst at any point, just giving it a new
register number.

Thanks for the review!