[Mesa-dev] [PATCH 1/2] i965: Implement b2f and b2i using negation.

[Matt Turner]

On Fri, Jul 10, 2015 at 10:06 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Booleans are represented as 0/-1 on modern hardware which means we can
> just negate them to convert them into a numeric type.  Negation has
> the benefit that it can be implemented using a source modifier which
> can easily be propagated into some other instruction.  shader-db
> results on HSW:
>
>  total instructions in shared programs: 5264246 -> 5264211 (-0.00%)
>  instructions in affected programs:     1464 -> 1429 (-2.39%)
>  helped:                                15
>  HURT:                                  1

Strange, I get different (better) numbers on Haswell:

total instructions in shared programs: 6279705 -> 6277316 (-0.04%)
instructions in affected programs:     40948 -> 38559 (-5.83%)
helped:                                123
HURT:                                  1
GAINED:                                1
LOST:                                  0

Certainly more than 15 helped programs in Civilization Beyond Earth alone.

The one hurt program is
rocketbirds-hardboiled-chicken/fp-2.shader_test, which is hurt because
we do not CSE the MOV instructions. I'll send a patch to fix this.

> No piglit regressions.

As a rule, this is implied by sending the patch. Don't put it in the
commit log -- in the worst case the patch is rebased and it's no
longer true (this has happened, embarrassingly enough). Same thing in
2/2.

> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp       | 4 +---
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 7 +------
>  2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 4690d00..64ff24c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -969,10 +969,8 @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr *instr)
>        break;
>
>     case nir_op_b2i:
> -      bld.AND(result, op[0], fs_reg(1));
> -      break;
>     case nir_op_b2f:
> -      bld.AND(retype(result, BRW_REGISTER_TYPE_UD), op[0], fs_reg(0x3f800000u));
> +      bld.MOV(result, negate(op[0]));
>        break;
>
>     case nir_op_f2b:
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index c9c2661..fd94a70 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -1733,16 +1733,11 @@ vec4_visitor::visit(ir_expression *ir)
>        emit(MOV(result_dst, op[0]));
>        break;
>     case ir_unop_b2i:
> -      emit(AND(result_dst, op[0], src_reg(1)));
> -      break;
>     case ir_unop_b2f:
>        if (devinfo->gen <= 5) {
>           resolve_bool_comparison(ir->operands[0], &op[0]);
>        }
> -      op[0].type = BRW_REGISTER_TYPE_D;
> -      result_dst.type = BRW_REGISTER_TYPE_D;
> -      emit(AND(result_dst, op[0], src_reg(0x3f800000u)));
> -      result_dst.type = BRW_REGISTER_TYPE_F;
> +      emit(MOV(result_dst, negate(op[0])));
>        break;
>     case ir_unop_f2b:
>        emit(CMP(result_dst, op[0], src_reg(0.0f), BRW_CONDITIONAL_NZ));
> --
> 2.4.3
>

Good idea. Not sure why I didn't think of that before.

Both are:

Reviewed-by: Matt Turner <mattst88 at gmail.com>