[Mesa-dev] [PATCH 1/3] i965/fs: Reimplement dead_code_elimination().

[Eric Anholt]

Matt Turner <mattst88 at gmail.com> writes:
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
> new file mode 100644
> index 0000000..6addbb3
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp

> +
> +/** @file brw_fs_dead_code_eliminate.cpp
> + */

Perhaps some actual comment:

* Dataflow-aware dead code elimination.
*
* Walks the instruction list from the bottom, removing instructions that
* have results that both aren't used in later blocks and haven't been read
* yet in the tail end of this block.

> +bool
> +fs_visitor::dead_code_eliminate()
> +{
> +   bool progress = false;
> +
> +   cfg_t cfg(&instructions);
> +
> +   calculate_live_intervals();
> +
> +   int num_vars = live_intervals->num_vars;
> +   BITSET_WORD *live = ralloc_array(NULL, BITSET_WORD, BITSET_WORDS(num_vars));
> +
> +   for (int b = 0; b < cfg.num_blocks; b++) {
> +      bblock_t *block = cfg.blocks[b];
> +      memcpy(live, live_intervals->bd[b].liveout,
> +             sizeof(BITSET_WORD) * BITSET_WORDS(num_vars));
> +
> +      for (fs_inst *inst = (fs_inst *)block->end;
> +           inst != block->start->prev;
> +           inst = (fs_inst *)inst->prev) {
> +         if (inst->dst.file == GRF &&
> +             !inst->has_side_effects() &&
> +             !inst->writes_flag()) {
> +            bool result_live = false;
> +
> +            if (inst->regs_written == 1) {
> +               int var = live_intervals->var_from_reg(&inst->dst);
> +               result_live = BITSET_TEST(live, var);
> +            } else {
> +               int var = live_intervals->var_from_vgrf[inst->dst.reg];
> +               for (int i = 0; i < inst->regs_written; i++) {
> +                  result_live = result_live || BITSET_TEST(live, var + i);
> +               }
> +            }

You could just use the else block of this if statement all the time,
right?  Seems easier.

> +            if (!result_live) {
> +               progress = true;
> +
> +               switch (inst->opcode) {
> +               case BRW_OPCODE_ADDC:
> +               case BRW_OPCODE_SUBB:
> +               case BRW_OPCODE_MACH:
> +                  inst->dst = fs_reg(retype(brw_null_reg(), inst->dst.type));
> +                  break;
> +               default:
> +                  inst->opcode = BRW_OPCODE_NOP;
> +                  break;
> +               }
> +               continue;

I don't think the continue here is quite right: you'll skip looking at
the src args for a nulled-destination ADDC/SUBB/MACH.  I think the
continue would still be appropriate in the default case.

> +            }
> +         }
> +
> +         for (int i = 0; i < 3; i++) {
> +            if (inst->src[i].file == GRF) {
> +               int var = live_intervals->var_from_vgrf[inst->src[i].reg];
> +
> +               for (int j = 0; j < inst->regs_read(this, i); j++) {
> +                  BITSET_SET(live, var + inst->src[i].reg_offset + j);
> +               }
> +            }
> +         }
> +      }
> +   }
> +
> +   ralloc_free(live);
> +
> +   foreach_list_safe(node, &this->instructions) {
> +      fs_inst *inst = (fs_inst *)node;
> +
> +      if (inst->opcode == BRW_OPCODE_NOP) {
> +         inst->remove();
> +      }
> +   }

Tiny optimization: this block could go under if (progress).

> +   if (progress)
> +      invalidate_live_intervals();
> +
> +   return progress;

The "continue" comment is the only important one, I think.  Anything
else you can take or leave, and this series is:

Reviewed-by: Eric Anholt <eric at anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140414/0a2a6254/attachment.sig>