[Mesa-dev] [PATCH 4/5] i965/fs: Dead code eliminate instructions writing the flag.

[Kenneth Graunke]

On Wednesday, October 29, 2014 02:10:12 PM Matt Turner wrote:
> Most prominently helps Natural Selection 2, which has a surprising
> number shaders that do very complicated things before drawing black.
> 
> instructions in affected programs:     23824 -> 19570 (-17.86%)
> ---
>  .../dri/i965/brw_fs_dead_code_eliminate.cpp        | 23 +++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

(requoting the diff to add more context...)

> 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 index
> 9cf8d89..c5f5ede 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
> @@ -21,93 +21,111 @@
> 
>   * IN THE SOFTWARE.
>   */
>  
>  #include "brw_fs.h"
>  #include "brw_fs_live_variables.h"
>  #include "brw_cfg.h"
>  
>  /** @file brw_fs_dead_code_eliminate.cpp
>   *
>   * 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;
>     
>     calculate_live_intervals();
>     
>     int num_vars = live_intervals->num_vars;
>     BITSET_WORD *live = ralloc_array(NULL, BITSET_WORD,
>     BITSET_WORDS(num_vars));
> +   BITSET_WORD *flag_live = ralloc_array(NULL, BITSET_WORD, 1);
> 
>     foreach_block (block, cfg) {    
>        memcpy(live, live_intervals->block_data[block->num].liveout,       
>               sizeof(BITSET_WORD) * BITSET_WORDS(num_vars));
> +      memcpy(flag_live, live_intervals->block_data[block->num].flag_liveout,
> +             sizeof(BITSET_WORD));
> 
>        foreach_inst_in_block_reverse(fs_inst, inst, block) {
> -         if (inst->dst.file == GRF &&
> -             !inst->has_side_effects() &&
> -             !inst->writes_flag()) {
> +         if (inst->dst.file == GRF && !inst->has_side_effects()) {
>              bool result_live = false;

This seems wrong to me.  Instructions handled here must have a destination, but now
can also write the flag...yet...

>              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_reg(&inst->dst);
>                 for (int i = 0; i < inst->regs_written; i++) {
>                    result_live = result_live || BITSET_TEST(live, var + i);
>                 }
>              }
>
>              if (!result_live) {
>                 progress = true;
>
>                 if (inst->writes_accumulator) {
>                    inst->dst = fs_reg(retype(brw_null_reg(),
>                    inst->dst.type));
>                 } else {
>                    inst->opcode = BRW_OPCODE_NOP;
>                    continue;

...here you NOP the instruction, without considering whether the flag value is live.

I think you meant to change the (inst->writes_accumulator check to be

   if (inst->writes_accumulator || inst->writes_flags())

that way, you just make the destination NULL, but leave it generating the flag register...

>                 }
>              }
>           }
> 
> +         if (inst->dst.is_null() && inst->writes_flag()) {
> +            if (!BITSET_TEST(flag_live, inst->flag_subreg)) {
> +               inst->opcode = BRW_OPCODE_NOP;
> +               progress = true;
> +               continue;
> +            }
> +         }

...which this code block would clean up, NOP'ing instructions with a NULL destination
and unused flag value.

With that fixed, it looks good to me.
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +
> 
>           if (inst->dst.file == GRF) {
>              if (!inst->is_partial_write()) {
>                 int var = live_intervals->var_from_reg(&inst->dst);
>                 for (int i = 0; i < inst->regs_written; i++) {
>                    BITSET_CLEAR(live, var + i);
>                 }
>              }
>           }
> 
> +         if (inst->writes_flag()) {
> +            BITSET_CLEAR(flag_live, inst->flag_subreg);
> +         }
> +
> 
>           for (int i = 0; i < inst->sources; i++) {
>              if (inst->src[i].file == GRF) {
>                 int var = live_intervals->var_from_reg(&inst->src[i]);
>                 for (int j = 0; j < inst->regs_read(this, i); j++) {
>                    BITSET_SET(live, var + j);
>                 }
>              }
>           }
> 
> +
> +         if (inst->reads_flag()) {
> +            BITSET_SET(flag_live, inst->flag_subreg);
> +         }
>        }
>     }
>     
>     ralloc_free(live);
> +   ralloc_free(flag_live);
> 
>     if (progress) {
>        foreach_block_and_inst_safe (block, backend_instruction, inst, cfg) {
>           if (inst->opcode == BRW_OPCODE_NOP) {
>              inst->remove(block);
>           }
>        }
>        
>        invalidate_live_intervals();
>     }
>     
>     return progress;
>  }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141108/2ac54565/attachment-0001.sig>