[Mesa-dev] [PATCH 4/9] i965/fs: Get rid of all remaining algebraic optimizations for floats

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 74 +++++++++---------------------------
>  1 file changed, 18 insertions(+), 56 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 365231e..9f303d5 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2097,28 +2097,27 @@ fs_visitor::opt_algebraic()
>     bool progress = false;
>  
>     foreach_block_and_inst(block, fs_inst, inst, cfg) {
> -      switch (inst->opcode) {
> -      case BRW_OPCODE_MOV:
> -         if (inst->src[0].file != IMM)
> -            break;
> -
> -         if (inst->saturate) {
> -            if (inst->dst.type != inst->src[0].type)
> -               assert(!"unimplemented: saturate mixed types");
> +      /* We don't want to do any float algebraic optimizations in the backend
> +       * because they aren't bit-for-bit safe and the backend doesn't know
> +       * when a value is declared precise or invariant.
> +       */
> +      if (inst->dst.type == BRW_REGISTER_TYPE_F)
> +         continue;
>  
> -            if (brw_saturate_immediate(inst->dst.type,
> -                                       &inst->src[0].as_brw_reg())) {
> -               inst->saturate = false;
> -               progress = true;
> -            }
> -         }
> -         break;
> +      bool has_float_src = false;
> +      for (unsigned i = 0; i < inst->sources; i++) {
> +         if (inst->src[i].type == BRW_REGISTER_TYPE_F)
> +            has_float_src = true;
> +      }
> +      if (has_float_src)
> +         continue;
>  
> +      switch (inst->opcode) {
>        case BRW_OPCODE_MUL:
>  	 if (inst->src[1].file != IMM)
>  	    continue;
>  
> -	 /* a * 1.0 = a */
> +	 /* a * 1 = a */
>  	 if (inst->src[1].is_one()) {
>  	    inst->opcode = BRW_OPCODE_MOV;
>  	    inst->src[1] = reg_undef;
> @@ -2126,7 +2125,7 @@ fs_visitor::opt_algebraic()
>  	    break;
>  	 }
>  
> -         /* a * -1.0 = -a */
> +         /* a * -1 = -a */
>           if (inst->src[1].is_negative_one()) {
>              inst->opcode = BRW_OPCODE_MOV;
>              inst->src[0].negate = !inst->src[0].negate;
> @@ -2135,7 +2134,7 @@ fs_visitor::opt_algebraic()
>              break;
>           }
>  
> -         /* a * 0.0 = 0.0 */
> +         /* a * 0 = 0 */
>           if (inst->src[1].is_zero()) {
>              inst->opcode = BRW_OPCODE_MOV;
>              inst->src[0] = inst->src[1];
> @@ -2157,7 +2156,7 @@ fs_visitor::opt_algebraic()
>           if (inst->src[1].file != IMM)
>              continue;
>  
> -         /* a + 0.0 = a */
> +         /* a + 0 = a */
>           if (inst->src[1].is_zero()) {
>              inst->opcode = BRW_OPCODE_MOV;
>              inst->src[1] = reg_undef;
> @@ -2182,16 +2181,6 @@ fs_visitor::opt_algebraic()
>              break;
>           }
>           break;
> -      case BRW_OPCODE_LRP:
> -         if (inst->src[1].equals(inst->src[2])) {
> -            inst->opcode = BRW_OPCODE_MOV;
> -            inst->src[0] = inst->src[1];
> -            inst->src[1] = reg_undef;
> -            inst->src[2] = reg_undef;
> -            progress = true;
> -            break;
> -         }
> -         break;
>        case BRW_OPCODE_CMP:
>           if (inst->conditional_mod == BRW_CONDITIONAL_GE &&
>               inst->src[0].abs &&
> @@ -2213,33 +2202,6 @@ fs_visitor::opt_algebraic()
>              progress = true;
>           }
>           break;
> -      case BRW_OPCODE_MAD:
> -         if (inst->src[1].is_zero() || inst->src[2].is_zero()) {
> -            inst->opcode = BRW_OPCODE_MOV;
> -            inst->src[1] = reg_undef;
> -            inst->src[2] = reg_undef;
> -            progress = true;
> -         } else if (inst->src[0].is_zero()) {
> -            inst->opcode = BRW_OPCODE_MUL;
> -            inst->src[0] = inst->src[2];
> -            inst->src[2] = reg_undef;
> -            progress = true;
> -         } else if (inst->src[1].is_one()) {
> -            inst->opcode = BRW_OPCODE_ADD;
> -            inst->src[1] = inst->src[2];
> -            inst->src[2] = reg_undef;
> -            progress = true;
> -         } else if (inst->src[2].is_one()) {
> -            inst->opcode = BRW_OPCODE_ADD;
> -            inst->src[2] = reg_undef;
> -            progress = true;
> -         } else if (inst->src[1].file == IMM && inst->src[2].file == IMM) {
> -            inst->opcode = BRW_OPCODE_ADD;
> -            inst->src[1].f *= inst->src[2].f;
> -            inst->src[2] = reg_undef;
> -            progress = true;
> -         }
> -         break;

Most of these seem invariant- and precise-safe (except a * 0 = 0 and the
MAD ones that rely on the same equality), and it doesn't save a huge lot
of code in the back-end to get rid of them.  How about we only remove
the ones that actually break invariance for the moment?

In the long term it's likely to make sense to plumb invariance
information through the i965 back-end so we only avoid these on precise
operations.  It shouldn't be hard to do, but the fact that you observed
so little shader-db regressions by just removing them seems like a
convincing enough argument for holding ourselves back from doing the
work for now.

>        case SHADER_OPCODE_BROADCAST:
>           if (is_uniform(inst->src[0])) {
>              inst->opcode = BRW_OPCODE_MOV;
> -- 
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160317/6c7482f2/attachment.sig>