[Mesa-dev] [PATCH 14/20] i965/vec4: Don't iterate between blocks with inst->next/prev.

[Pohjolainen, Topi]

On Tue, Sep 02, 2014 at 09:34:25PM -0700, Matt Turner wrote:
> The register coalescing portion of this patch hurts three shaders in
> Guacamelee by one instruction each, but examining the diff makes me
> believe that what we were generating was (perhaps harmlessly) incorrect.
> ---
>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index 6669281..e3869d6 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -513,12 +513,9 @@ vec4_visitor::dead_code_eliminate()
>           }
>        }
>  
> -      for (exec_node *node = inst->prev, *prev = node->prev;
> -           prev != NULL && dead_channels != 0;
> -           node = prev, prev = prev->prev) {
> -         vec4_instruction *scan_inst = (vec4_instruction  *)node;
> -
> -         if (scan_inst->is_control_flow())

Last instruction of the block is not considered in the iteration, but first
instruction is. Hence if I'm reading this right, before DO and ENDIF weren't
considered but now they are.

> +      foreach_inst_in_block_reverse_starting_from(vec4_instruction, scan_inst,
> +                                                  inst, block) {
> +         if (dead_channels == 0)
>              break;
>  
>           if (inst_writes_flag) {
> @@ -1059,10 +1056,11 @@ vec4_visitor::opt_register_coalesce()
>         * everything writing to the temporary to write into the destination
>         * instead.
>         */
> -      vec4_instruction *scan_inst;
> -      for (scan_inst = (vec4_instruction *)inst->prev;
> -	   scan_inst->prev != NULL;
> -	   scan_inst = (vec4_instruction *)scan_inst->prev) {
> +      vec4_instruction *_scan_inst = (vec4_instruction *)inst->prev;
> +      foreach_inst_in_block_reverse_starting_from(vec4_instruction, scan_inst,
> +                                                  inst, block) {
> +         _scan_inst = scan_inst;
> +
>  	 if (scan_inst->dst.file == GRF &&
>  	     scan_inst->dst.reg == inst->src[0].reg &&
>  	     scan_inst->dst.reg_offset == inst->src[0].reg_offset) {
> @@ -1104,16 +1102,6 @@ vec4_visitor::opt_register_coalesce()
>  	       break;
>  	 }
>  
> -	 /* We don't handle flow control here.  Most computation of values
> -	  * that could be coalesced happens just before their use.
> -	  */
> -	 if (scan_inst->opcode == BRW_OPCODE_DO ||
> -	     scan_inst->opcode == BRW_OPCODE_WHILE ||
> -	     scan_inst->opcode == BRW_OPCODE_ELSE ||
> -	     scan_inst->opcode == BRW_OPCODE_ENDIF) {
> -	    break;
> -	 }

The same reasoning here.

> -
>           /* You can't read from an MRF, so if someone else reads our MRF's
>            * source GRF that we wanted to rewrite, that stops us.  If it's a
>            * GRF we're trying to coalesce to, we don't actually handle
> @@ -1166,7 +1154,7 @@ vec4_visitor::opt_register_coalesce()
>  	  * computing the value.  Now go rewrite the instruction stream
>  	  * between the two.
>  	  */
> -
> +         vec4_instruction *scan_inst = _scan_inst;
>  	 while (scan_inst != inst) {
>  	    if (scan_inst->dst.file == GRF &&
>  		scan_inst->dst.reg == inst->src[0].reg &&
> -- 
> 1.8.5.5
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev