[Mesa-dev] [PATCH 04/10] i965/fs: Pull ir_binop_min/ir_binop_max handling to a separate function.

[Kenneth Graunke]

On 09/22/2012 02:04 PM, Eric Anholt wrote:
> This will be reused from the ARB_fp compiler.  I touched up the pre-gen6 path
> to not overwrite dst in the first instruction, which prevents the need for
> aliasing checks (we'll need that in the ARB_fp compiler, but it actually
> hasn't been needed in this codebase since the revert of the nasty old
> MOV-avoidance code)
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h           |    2 +
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |   52 +++++++++++---------------
>  2 files changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 5134857..9cb9590 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -317,6 +317,8 @@ public:
>  			      fs_reg shadow_comp, fs_reg lod, fs_reg lod2);
>     fs_inst *emit_math(enum opcode op, fs_reg dst, fs_reg src0);
>     fs_inst *emit_math(enum opcode op, fs_reg dst, fs_reg src0, fs_reg src1);
> +   void emit_minmax(uint32_t conditionalmod, fs_reg dst,
> +                    fs_reg src0, fs_reg src1);
>     bool try_emit_saturate(ir_expression *ir);
>     bool try_emit_mad(ir_expression *ir, int mul_arg);
>     void emit_bool_to_cond_code(ir_rvalue *condition);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 5bd7238..c8d976f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -178,6 +178,24 @@ fs_visitor::visit(ir_dereference_array *ir)
>     }
>  }
>  
> +void
> +fs_visitor::emit_minmax(uint32_t conditionalmod, fs_reg dst,
> +                        fs_reg src0, fs_reg src1)
> +{
> +   fs_inst *inst;
> +
> +   if (intel->gen >= 6) {
> +      inst = emit(BRW_OPCODE_SEL, dst, src0, src1);
> +      inst->conditional_mod = conditionalmod;
> +   } else {
> +      inst = emit(BRW_OPCODE_CMP, reg_null_cmp, src0, src1);
> +      inst->conditional_mod = conditionalmod;
> +
> +      inst = emit(BRW_OPCODE_SEL, dst, src0, src1);
> +      inst->predicated = true;
> +   }
> +}
> +
>  /* Instruction selection: Produce a MOV.sat instead of
>   * MIN(MAX(val, 0), 1) when possible.
>   */
> @@ -515,40 +533,12 @@ fs_visitor::visit(ir_expression *ir)
>        break;
>  
>     case ir_binop_min:
> -      resolve_ud_negate(&op[0]);
> -      resolve_ud_negate(&op[1]);
> -
> -      if (intel->gen >= 6) {
> -	 inst = emit(BRW_OPCODE_SEL, this->result, op[0], op[1]);
> -	 inst->conditional_mod = BRW_CONDITIONAL_L;
> -      } else {
> -	 /* Unalias the destination */
> -	 this->result = fs_reg(this, ir->type);
> -
> -	 inst = emit(BRW_OPCODE_CMP, this->result, op[0], op[1]);
> -	 inst->conditional_mod = BRW_CONDITIONAL_L;
> -
> -	 inst = emit(BRW_OPCODE_SEL, this->result, op[0], op[1]);
> -	 inst->predicated = true;
> -      }
> -      break;
>     case ir_binop_max:
>        resolve_ud_negate(&op[0]);
>        resolve_ud_negate(&op[1]);
> -
> -      if (intel->gen >= 6) {
> -	 inst = emit(BRW_OPCODE_SEL, this->result, op[0], op[1]);
> -	 inst->conditional_mod = BRW_CONDITIONAL_GE;
> -      } else {
> -	 /* Unalias the destination */
> -	 this->result = fs_reg(this, ir->type);
> -
> -	 inst = emit(BRW_OPCODE_CMP, this->result, op[0], op[1]);
> -	 inst->conditional_mod = BRW_CONDITIONAL_G;
> -
> -	 inst = emit(BRW_OPCODE_SEL, this->result, op[0], op[1]);
> -	 inst->predicated = true;
> -      }
> +      emit_minmax(ir->operation == ir_binop_min ?
> +                  BRW_CONDITIONAL_L : BRW_CONDITIONAL_GE,
> +                  this->result, op[0], op[1]);
>        break;
>  
>     case ir_binop_pow:

Noticed a small change here: we used to use > on Gen4-5, and your patch
makes us use >=.  It shouldn't matter...if both operands are equal,
either one is an acceptable result.  The added consistency is nice.

You could make that a separate patch (so it's a clean refactor) but I
don't really care that much.