[Mesa-dev] [PATCH v2 28/53] intel/compiler: add new half-float register type for 3-src instructions

[Pohjolainen, Topi]

On Thu, Jan 03, 2019 at 07:50:03AM +0100, Iago Toral wrote:
> On Wed, 2019-01-02 at 11:35 +0200, Pohjolainen, Topi wrote:
> > On Wed, Dec 19, 2018 at 12:50:56PM +0100, Iago Toral Quiroga wrote:
> > > This is available since gen8.
> > > ---
> > >  src/intel/compiler/brw_reg_type.c | 35
> > > +++++++++++++++++++++++++++----
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/intel/compiler/brw_reg_type.c
> > > b/src/intel/compiler/brw_reg_type.c
> > > index 60240ba1513..72295a2bd75 100644
> > > --- a/src/intel/compiler/brw_reg_type.c
> > > +++ b/src/intel/compiler/brw_reg_type.c
> > > @@ -138,6 +138,7 @@ enum hw_3src_reg_type {
> > >     GEN7_3SRC_TYPE_D  = 1,
> > >     GEN7_3SRC_TYPE_UD = 2,
> > >     GEN7_3SRC_TYPE_DF = 3,
> > > +   GEN8_3SRC_TYPE_HF = 4,
> > >  
> > >     /** When ExecutionDatatype is 1: @{ */
> > >     GEN10_ALIGN1_3SRC_REG_TYPE_HF = 0b000,
> > > @@ -166,6 +167,14 @@ static const struct hw_3src_type {
> > >     [BRW_REGISTER_TYPE_D]  = { GEN7_3SRC_TYPE_D  },
> > >     [BRW_REGISTER_TYPE_UD] = { GEN7_3SRC_TYPE_UD },
> > >     [BRW_REGISTER_TYPE_DF] = { GEN7_3SRC_TYPE_DF },
> > > +}, gen8_hw_3src_type[] = {
> > > +   [0 ... BRW_REGISTER_TYPE_LAST] = { INVALID },
> > > +
> > > +   [BRW_REGISTER_TYPE_F]  = { GEN7_3SRC_TYPE_F  },
> > > +   [BRW_REGISTER_TYPE_D]  = { GEN7_3SRC_TYPE_D  },
> > > +   [BRW_REGISTER_TYPE_UD] = { GEN7_3SRC_TYPE_UD },
> > > +   [BRW_REGISTER_TYPE_DF] = { GEN7_3SRC_TYPE_DF },
> > > +   [BRW_REGISTER_TYPE_HF] = { GEN8_3SRC_TYPE_HF },
> > >  }, gen10_hw_3src_align1_type[] = {
> > >  #define E(x) BRW_ALIGN1_3SRC_EXEC_TYPE_##x
> > >     [0 ... BRW_REGISTER_TYPE_LAST] = { INVALID },
> > > @@ -249,6 +258,20 @@ brw_hw_type_to_reg_type(const struct
> > > gen_device_info *devinfo,
> > >     unreachable("not reached");
> > >  }
> > >  
> > > +static inline const struct hw_3src_type *
> > > +get_hw_3src_type_map(const struct gen_device_info *devinfo,
> > > uint32_t *size)
> > > +{
> > > +   if (devinfo->gen < 8) {
> > > +      if (size)
> > > +         *size = ARRAY_SIZE(gen7_hw_3src_type);
> > > +      return gen7_hw_3src_type;
> > > +   } else {
> > > +      if (size)
> > > +         *size = ARRAY_SIZE(gen8_hw_3src_type);
> > > +      return gen8_hw_3src_type;
> > > +   }
> > > +}
> > > +
> > >  /**
> > >   * Convert a brw_reg_type enumeration value into the hardware
> > > representation
> > >   * for a 3-src align16 instruction
> > > @@ -257,9 +280,11 @@ unsigned
> > >  brw_reg_type_to_a16_hw_3src_type(const struct gen_device_info
> > > *devinfo,
> > >                                   enum brw_reg_type type)
> > >  {
> > > -   assert(type < ARRAY_SIZE(gen7_hw_3src_type));
> > > -   assert(gen7_hw_3src_type[type].reg_type != (enum
> > > hw_3src_reg_type)INVALID);
> > > -   return gen7_hw_3src_type[type].reg_type;
> > > +   uint32_t map_size;
> > > +   const struct hw_3src_type *hw_3src_type_map =
> > > +      get_hw_3src_type_map(devinfo, &map_size);
> > > +   assert(hw_3src_type_map[type].reg_type != (enum
> > > hw_3src_reg_type)INVALID);
> > > +   return hw_3src_type_map[type].reg_type;
> > 
> > I wonder if we should use a style equivalent to
> > brw_reg_type_to_hw_type() and
> > brw_hw_type_to_reg_type() and inline the table (or map) selection:
> 
> I don't have a strong opinion, but since we need this in at least two
> different places I think it is best to have that code in a single
> function that we can reuse rather than replicating it wherever we need
> it. I'd be more in favor of changing the other functions to follow a
> similar pattern for the same reason.

I don't have a preference either. Having similar logic in both would be nice
though. We could, for example, go with your patch here and as a follow-up
modify the existing. Hence, this patch:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> 
> Iago
> 
> >       const struct hw_type *table;
> > 
> >       if (devinfo->gen >= 8) {
> >          assert(type < ARRAY_SIZE(gen8_hw_3src_type));
> >          table = gen7_hw_3src_type;
> >       } else {
> >          assert(type < ARRAY_SIZE(gen7_hw_3src_type));
> >          table = gen7_hw_3src_type;
> >       }
> > 
> >       assert(table[type].reg_type != (enum hw_reg_type)INVALID);
> > 
> >       return table[type].reg_type;
> > 
> > >  }
> > >  
> > >  /**
> > > @@ -283,8 +308,10 @@ enum brw_reg_type
> > >  brw_a16_hw_3src_type_to_reg_type(const struct gen_device_info
> > > *devinfo,
> > >                                   unsigned hw_type)
> > >  {
> > > +   const struct hw_3src_type *hw_3src_type_map =
> > > +      get_hw_3src_type_map(devinfo, NULL);
> > >     for (enum brw_reg_type i = 0; i <= BRW_REGISTER_TYPE_LAST; i++)
> > > {
> > > -      if (gen7_hw_3src_type[i].reg_type == hw_type) {
> > > +      if (hw_3src_type_map[i].reg_type == hw_type) {
> > >           return i;
> > >        }
> > >     }
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> > 
>