[Mesa-dev] [PATCH v2 10/18] intel/compiler: fix 16-bit comparisons

[Jason Ekstrand]

On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga <itoral at igalia.com>
wrote:

> NIR assumes that booleans are always 32-bit, but Intel hardware produces
> 16-bit booleans for 16-bit comparisons. This means that we need to convert
> the 16-bit result to 32-bit.
>
> In the future we want to add an optimization pass to clean this up and
> hopefully remove the conversions.
> ---
>  src/intel/compiler/brw_fs_nir.cpp | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index b9d8ade4cf..d590a00385 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -1017,9 +1017,13 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>     case nir_op_feq:
>     case nir_op_fne: {
>        fs_reg dest = result;
> -      if (nir_src_bit_size(instr->src[0].src) > 32) {
> +
> +      const uint32_t bit_size =  nir_src_bit_size(instr->src[0].src);
> +      if (bit_size > 32)
>           dest = bld.vgrf(BRW_REGISTER_TYPE_DF, 1);
> -      }
> +      else if (bit_size < 32)
> +         dest = bld.vgrf(BRW_REGISTER_TYPE_HF, 1);
>

This is going to break for 8-bit.  Maybe add an assert?  For that matter,
why not just use the type of src0 for the destination type?  How do 8-bit
comparisons work?  Do they return a 16-bit value?


> +
>        brw_conditional_mod cond;
>        switch (instr->op) {
>        case nir_op_flt:
> @@ -1037,9 +1041,17 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>        default:
>           unreachable("bad opcode");
>        }
> +
>        bld.CMP(dest, op[0], op[1], cond);
> -      if (nir_src_bit_size(instr->src[0].src) > 32) {
> +
> +      if (bit_size > 32) {
>           bld.MOV(result, subscript(dest, BRW_REGISTER_TYPE_UD, 0));
> +      } else if(bit_size < 32) {
> +         /* When we convert the result to 32-bit we need to be careful
> and do
> +          * it as a signed conversion to get sign extension (for 32-bit
> true)
> +          */
> +         bld.MOV(retype(result, BRW_REGISTER_TYPE_D),
> +                 retype(dest, BRW_REGISTER_TYPE_W));
>

Maybe better to use brw_reg_type_from_bit_size so 8-bit gets automatically
handled?


>        }
>        break;
>     }
> @@ -1051,9 +1063,12 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>     case nir_op_ieq:
>     case nir_op_ine: {
>        fs_reg dest = result;
> -      if (nir_src_bit_size(instr->src[0].src) > 32) {
> +
> +      const uint32_t bit_size = nir_src_bit_size(instr->src[0].src);
> +      if (bit_size > 32)
>           dest = bld.vgrf(BRW_REGISTER_TYPE_UQ, 1);
> -      }
> +      else if (bit_size < 32)
> +         dest = bld.vgrf(BRW_REGISTER_TYPE_W, 1);
>
>        brw_conditional_mod cond;
>        switch (instr->op) {
> @@ -1075,8 +1090,15 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>           unreachable("bad opcode");
>        }
>        bld.CMP(dest, op[0], op[1], cond);
> -      if (nir_src_bit_size(instr->src[0].src) > 32) {
> +
> +      if (bit_size > 32) {
>           bld.MOV(result, subscript(dest, BRW_REGISTER_TYPE_UD, 0));
> +      } else if (bit_size < 32) {
> +         /* When we convert the result to 32-bit we need to be careful
> and do
> +          * it as a signed conversion to get sign extension (for 32-bit
> true)
> +          */
> +         bld.MOV(retype(result, BRW_REGISTER_TYPE_D),
> +                 retype(dest, BRW_REGISTER_TYPE_W));
>        }
>        break;
>     }
> --
> 2.14.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180430/39c3d20c/attachment.html>