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

[Iago Toral]

On Mon, 2018-04-30 at 14:43 -0700, Jason Ekstrand wrote:
> On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga <itoral at igalia.co
> m> 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?

According to the PRM a Byte CMP writes 0xFF for true, so they return an
8-bit value. I think using the type of the source operand here should
work.
>  
> > +
> > 
> >        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?

Ah yes, that's a good idea, thanks!
Iago
> >        }
> > 
> >        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/20180502/b27d32ed/attachment-0001.html>