On Mon, Apr 14, 2014 at 5:59 PM, Eric Anholt <e...@anholt.net> wrote:
> Matt Turner <matts...@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 <e...@anholt.net>
Thanks Eric!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
