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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
