Re: [Mesa-dev] [PATCH v2 28/53] intel/compiler: add new half-float register type for 3-src instructions
On Thu, 2019-01-03 at 10:25 +0200, Pohjolainen, Topi wrote:
> 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, _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
Sure, I'll write that patch.
Iago
> >
> > 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
Re: [Mesa-dev] [PATCH v2 28/53] intel/compiler: add new half-float register type for 3-src instructions
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, _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
>
> 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@lists.freedesktop.org
Re: [Mesa-dev] [PATCH v2 28/53] intel/compiler: add new half-float register type for 3-src instructions
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, _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.
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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 28/53] intel/compiler: add new half-float register type for 3-src instructions
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, _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:
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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
