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

[Matt Turner]

On Mon, Apr 14, 2014 at 5:59 PM, Eric Anholt <eric at anholt.net> wrote:
> 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.

Thanks, will add.

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

I don't think so, because of destinations with reg_offset != 0.

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

Yep, good catch.

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

Good idea.

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

Thanks Eric!