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

[Connor Abbott]

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

> +          !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.

> +            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.

> +         progress = true;
> +      }
> +   }
> +
> +   if (progress) {
> +      invalidate_live_intervals();
> +
> +      for (unsigned i = 0; i < ARRAY_SIZE(delta_x); i++) {
> +         if (delta_x[i].file == GRF && remap[delta_x[i].reg] != -1) {
> +            delta_x[i].reg = remap[delta_x[i].reg];
> +         }
> +      }
> +      for (unsigned i = 0; i < ARRAY_SIZE(delta_y); i++) {
> +         if (delta_y[i].file == GRF && remap[delta_y[i].reg] != -1) {
> +            delta_y[i].reg = remap[delta_y[i].reg];
> +         }
> +      }
> +   }
> +
> +   return progress;
> +}
> +
> +bool
>  fs_visitor::compute_to_mrf()
>  {
>     bool progress = false;
> @@ -3092,6 +3158,7 @@ fs_visitor::run()
>           OPT(dead_code_eliminate);
>           OPT(opt_peephole_sel);
>           OPT(dead_control_flow_eliminate, this);
> +         OPT(opt_register_renaming);
>           OPT(opt_saturate_propagation);
>           OPT(register_coalesce);
>           OPT(compute_to_mrf);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index e7a82c4..d30b0b8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -339,6 +339,7 @@ public:
>     bool opt_copy_propagate_local(void *mem_ctx, bblock_t *block,
>                                   exec_list *acp);
>     void opt_drop_redundant_mov_to_flags();
> +   bool opt_register_renaming();
>     bool register_coalesce();
>     bool compute_to_mrf();
>     bool dead_code_eliminate();
> --
> 1.8.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Other than my comments above,

Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>