Re: [Mesa-dev] [PATCH v3 12/43] i965/fs: Add brw_reg_type_from_bit_size utility method

[Chema Casanova]

El 30/10/17 a las 23:15, Jason Ekstrand escribió:
>
> On Mon, Oct 30, 2017 at 3:08 PM, Jason Ekstrand  > wrote:
>
> On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo
> > wrote:
>
> From: Alejandro Piñeiro  >
>
> Returns the brw_type for a given ssa.bit_size, and a reference
> type.
> So if bit_size is 64, and the reference type is
> BRW_REGISTER_TYPE_F,
> it returns BRW_REGISTER_TYPE_DF. The same applies if bit_size
> is 32
> and reference type is BRW_REGISTER_TYPE_HF it returns
> BRW_REGISTER_TYPE_F
>
> v2 (Jason Ekstrand):
>  - Use better unreachable() messages
>  - Add Q types
>
> Signed-off-by: Jose Maria Casanova Crespo
> >
> Signed-off-by: Alejandro Piñeiro  
> Reviewed-by: Jason Ekstrand  >
> ---
>  src/intel/compiler/brw_fs_nir.cpp | 69
> ---
>  1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7ed44f534c..affe65d5e9 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -227,6 +227,65 @@ fs_visitor::nir_emit_system_values()
>     }
>  }
>
> +/*
> + * Returns a type based on a reference_type (word, float,
> half-float) and a
> + * given bit_size.
> + *
> + * Reference BRW_REGISTER_TYPE are HF,F,DF,W,D,UW,UD.
> + *
> + * @FIXME: 64-bit return types are always DF on integer types
> to maintain
> + * compability with uses of DF previously to the introduction
> of int64
> + * support.
>
>
> I just read this comment and I really don't like it.  This is going to
> come back to bite us if we don't fix it some better way.  How many
> places do we actually need to override to DF?  I suppose we'll need it
> for intrinsics and a couple of ALU operations such as bcsel.  I'd like
> to keep it as contained as we can.

We have doubts about this behavior also that was the reason of the
@fixme. We created this function as we were using a similar switch in 4
places when giving the 16bit_storage support over the 64bits. You can
check the uses at the end of the patch. Two of them were originally
BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF the others where as expected
BRW_REGISTER_TYPE_F : BRW_REGISTER_TYPE_DF.

As Q types weren't used in this cases that was the reason to return DF
and avoid doing a retype with its conditional if needed, to not change
original code.

In any case, i will check if there are any regressions for this cases
changing the return types.

Thanks for the review.

Chema

>
> --Jason
>  
>
> + */
> +static brw_reg_type
> +brw_reg_type_from_bit_size(const unsigned bit_size,
> +                           const brw_reg_type reference_type)
> +{
> +   switch(reference_type) {
> +   case BRW_REGISTER_TYPE_HF:
> +   case BRW_REGISTER_TYPE_F:
> +   case BRW_REGISTER_TYPE_DF:
> +      switch(bit_size) {
> +      case 16:
> +         return BRW_REGISTER_TYPE_HF;
> +      case 32:
> +         return BRW_REGISTER_TYPE_F;
> +      case 64:
> +         return BRW_REGISTER_TYPE_DF;
> +      default:
> +         unreachable("Invalid bit size");
> +      }
> +   case BRW_REGISTER_TYPE_W:
> +   case BRW_REGISTER_TYPE_D:
> +   case BRW_REGISTER_TYPE_Q:
> +      switch(bit_size) {
> +      case 16:
> +         return BRW_REGISTER_TYPE_W;
> +      case 32:
> +         return BRW_REGISTER_TYPE_D;
> +      case 64:
> +         return BRW_REGISTER_TYPE_DF;
>
>
> This should be BRW_REGISTER_TYPE_Q
>  
>
> +      default:
> +         unreachable("Invalid bit size");
> +      }
> +   case BRW_REGISTER_TYPE_UW:
> +   case BRW_REGISTER_TYPE_UD:
> +   case BRW_REGISTER_TYPE_UQ:
> +      switch(bit_size) {
> +      case 16:
> +         return BRW_REGISTER_TYPE_UW;
> +      case 32:
> +         return BRW_REGISTER_TYPE_UD;
> +      case 64:
> +         return BRW_REGISTER_TYPE_DF;
>
>
> This should be BRW_REGISTER_TYPE_UQ
>
> With those fixed,
>
> Reviewed-by: Jason Ekstrand  >
>  
>

Re: [Mesa-dev] [PATCH v3 12/43] i965/fs: Add brw_reg_type_from_bit_size utility method

[Jason Ekstrand]

On Mon, Oct 30, 2017 at 3:08 PM, Jason Ekstrand 
wrote:

> On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo <
> jmcasan...@igalia.com> wrote:
>
>> From: Alejandro Piñeiro 
>>
>> Returns the brw_type for a given ssa.bit_size, and a reference type.
>> So if bit_size is 64, and the reference type is BRW_REGISTER_TYPE_F,
>> it returns BRW_REGISTER_TYPE_DF. The same applies if bit_size is 32
>> and reference type is BRW_REGISTER_TYPE_HF it returns BRW_REGISTER_TYPE_F
>>
>> v2 (Jason Ekstrand):
>>  - Use better unreachable() messages
>>  - Add Q types
>>
>> Signed-off-by: Jose Maria Casanova Crespo 
>> Signed-off-by: Alejandro Piñeiro > Reviewed-by: Jason Ekstrand 
>> ---
>>  src/intel/compiler/brw_fs_nir.cpp | 69 ++
>> ++---
>>  1 file changed, 64 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_fs_nir.cpp
>> b/src/intel/compiler/brw_fs_nir.cpp
>> index 7ed44f534c..affe65d5e9 100644
>> --- a/src/intel/compiler/brw_fs_nir.cpp
>> +++ b/src/intel/compiler/brw_fs_nir.cpp
>> @@ -227,6 +227,65 @@ fs_visitor::nir_emit_system_values()
>> }
>>  }
>>
>> +/*
>> + * Returns a type based on a reference_type (word, float, half-float)
>> and a
>> + * given bit_size.
>> + *
>> + * Reference BRW_REGISTER_TYPE are HF,F,DF,W,D,UW,UD.
>> + *
>> + * @FIXME: 64-bit return types are always DF on integer types to maintain
>> + * compability with uses of DF previously to the introduction of int64
>> + * support.
>>
>
I just read this comment and I really don't like it.  This is going to come
back to bite us if we don't fix it some better way.  How many places do we
actually need to override to DF?  I suppose we'll need it for intrinsics
and a couple of ALU operations such as bcsel.  I'd like to keep it as
contained as we can.

--Jason


> + */
>> +static brw_reg_type
>> +brw_reg_type_from_bit_size(const unsigned bit_size,
>> +   const brw_reg_type reference_type)
>> +{
>> +   switch(reference_type) {
>> +   case BRW_REGISTER_TYPE_HF:
>> +   case BRW_REGISTER_TYPE_F:
>> +   case BRW_REGISTER_TYPE_DF:
>> +  switch(bit_size) {
>> +  case 16:
>> + return BRW_REGISTER_TYPE_HF;
>> +  case 32:
>> + return BRW_REGISTER_TYPE_F;
>> +  case 64:
>> + return BRW_REGISTER_TYPE_DF;
>> +  default:
>> + unreachable("Invalid bit size");
>> +  }
>> +   case BRW_REGISTER_TYPE_W:
>> +   case BRW_REGISTER_TYPE_D:
>> +   case BRW_REGISTER_TYPE_Q:
>> +  switch(bit_size) {
>> +  case 16:
>> + return BRW_REGISTER_TYPE_W;
>> +  case 32:
>> + return BRW_REGISTER_TYPE_D;
>> +  case 64:
>> + return BRW_REGISTER_TYPE_DF;
>>
>
> This should be BRW_REGISTER_TYPE_Q
>
>
>> +  default:
>> + unreachable("Invalid bit size");
>> +  }
>> +   case BRW_REGISTER_TYPE_UW:
>> +   case BRW_REGISTER_TYPE_UD:
>> +   case BRW_REGISTER_TYPE_UQ:
>> +  switch(bit_size) {
>> +  case 16:
>> + return BRW_REGISTER_TYPE_UW;
>> +  case 32:
>> + return BRW_REGISTER_TYPE_UD;
>> +  case 64:
>> + return BRW_REGISTER_TYPE_DF;
>>
>
> This should be BRW_REGISTER_TYPE_UQ
>
> With those fixed,
>
> Reviewed-by: Jason Ekstrand 
>
>
>> +  default:
>> + unreachable("Invalid bit size");
>> +  }
>> +   default:
>> +  unreachable("Unknown type");
>> +   }
>> +}
>> +
>>  void
>>  fs_visitor::nir_emit_impl(nir_function_impl *impl)
>>  {
>> @@ -240,7 +299,7 @@ fs_visitor::nir_emit_impl(nir_function_impl *impl)
>>   reg->num_array_elems == 0 ? 1 : reg->num_array_elems;
>>unsigned size = array_elems * reg->num_components;
>>const brw_reg_type reg_type =
>> - reg->bit_size == 32 ? BRW_REGISTER_TYPE_F :
>> BRW_REGISTER_TYPE_DF;
>> + brw_reg_type_from_bit_size(reg->bit_size, BRW_REGISTER_TYPE_F);
>>nir_locals[reg->index] = bld.vgrf(reg_type, size);
>> }
>>
>> @@ -1341,7 +1400,7 @@ fs_visitor::nir_emit_load_const(const fs_builder
>> ,
>>  nir_load_const_instr *instr)
>>  {
>> const brw_reg_type reg_type =
>> -  instr->def.bit_size == 32 ? BRW_REGISTER_TYPE_D :
>> BRW_REGISTER_TYPE_DF;
>> +  brw_reg_type_from_bit_size(instr->def.bit_size,
>> BRW_REGISTER_TYPE_D);
>> fs_reg reg = bld.vgrf(reg_type, instr->def.num_components);
>>
>> switch (instr->def.bit_size) {
>> @@ -1369,8 +1428,8 @@ fs_visitor::get_nir_src(const nir_src )
>> fs_reg reg;
>> if (src.is_ssa) {
>>if (src.ssa->parent_instr->type == nir_instr_type_ssa_undef) {
>> - const brw_reg_type reg_type = src.ssa->bit_size == 32 ?
>> -BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF;
>> + const brw_reg_type reg_type =
>> +brw_reg_type_from_bit_size(src.ssa->bit_size,
>> BRW_REGISTER_TYPE_D);
>>   reg = 

Re: [Mesa-dev] [PATCH v3 12/43] i965/fs: Add brw_reg_type_from_bit_size utility method

[Jason Ekstrand]

On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo <
jmcasan...@igalia.com> wrote:

> From: Alejandro Piñeiro 
>
> Returns the brw_type for a given ssa.bit_size, and a reference type.
> So if bit_size is 64, and the reference type is BRW_REGISTER_TYPE_F,
> it returns BRW_REGISTER_TYPE_DF. The same applies if bit_size is 32
> and reference type is BRW_REGISTER_TYPE_HF it returns BRW_REGISTER_TYPE_F
>
> v2 (Jason Ekstrand):
>  - Use better unreachable() messages
>  - Add Q types
>
> Signed-off-by: Jose Maria Casanova Crespo 
> Signed-off-by: Alejandro Piñeiro  Reviewed-by: Jason Ekstrand 
> ---
>  src/intel/compiler/brw_fs_nir.cpp | 69 ++
> ++---
>  1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7ed44f534c..affe65d5e9 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -227,6 +227,65 @@ fs_visitor::nir_emit_system_values()
> }
>  }
>
> +/*
> + * Returns a type based on a reference_type (word, float, half-float) and
> a
> + * given bit_size.
> + *
> + * Reference BRW_REGISTER_TYPE are HF,F,DF,W,D,UW,UD.
> + *
> + * @FIXME: 64-bit return types are always DF on integer types to maintain
> + * compability with uses of DF previously to the introduction of int64
> + * support.
> + */
> +static brw_reg_type
> +brw_reg_type_from_bit_size(const unsigned bit_size,
> +   const brw_reg_type reference_type)
> +{
> +   switch(reference_type) {
> +   case BRW_REGISTER_TYPE_HF:
> +   case BRW_REGISTER_TYPE_F:
> +   case BRW_REGISTER_TYPE_DF:
> +  switch(bit_size) {
> +  case 16:
> + return BRW_REGISTER_TYPE_HF;
> +  case 32:
> + return BRW_REGISTER_TYPE_F;
> +  case 64:
> + return BRW_REGISTER_TYPE_DF;
> +  default:
> + unreachable("Invalid bit size");
> +  }
> +   case BRW_REGISTER_TYPE_W:
> +   case BRW_REGISTER_TYPE_D:
> +   case BRW_REGISTER_TYPE_Q:
> +  switch(bit_size) {
> +  case 16:
> + return BRW_REGISTER_TYPE_W;
> +  case 32:
> + return BRW_REGISTER_TYPE_D;
> +  case 64:
> + return BRW_REGISTER_TYPE_DF;
>

This should be BRW_REGISTER_TYPE_Q


> +  default:
> + unreachable("Invalid bit size");
> +  }
> +   case BRW_REGISTER_TYPE_UW:
> +   case BRW_REGISTER_TYPE_UD:
> +   case BRW_REGISTER_TYPE_UQ:
> +  switch(bit_size) {
> +  case 16:
> + return BRW_REGISTER_TYPE_UW;
> +  case 32:
> + return BRW_REGISTER_TYPE_UD;
> +  case 64:
> + return BRW_REGISTER_TYPE_DF;
>

This should be BRW_REGISTER_TYPE_UQ

With those fixed,

Reviewed-by: Jason Ekstrand 


> +  default:
> + unreachable("Invalid bit size");
> +  }
> +   default:
> +  unreachable("Unknown type");
> +   }
> +}
> +
>  void
>  fs_visitor::nir_emit_impl(nir_function_impl *impl)
>  {
> @@ -240,7 +299,7 @@ fs_visitor::nir_emit_impl(nir_function_impl *impl)
>   reg->num_array_elems == 0 ? 1 : reg->num_array_elems;
>unsigned size = array_elems * reg->num_components;
>const brw_reg_type reg_type =
> - reg->bit_size == 32 ? BRW_REGISTER_TYPE_F : BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(reg->bit_size, BRW_REGISTER_TYPE_F);
>nir_locals[reg->index] = bld.vgrf(reg_type, size);
> }
>
> @@ -1341,7 +1400,7 @@ fs_visitor::nir_emit_load_const(const fs_builder
> ,
>  nir_load_const_instr *instr)
>  {
> const brw_reg_type reg_type =
> -  instr->def.bit_size == 32 ? BRW_REGISTER_TYPE_D :
> BRW_REGISTER_TYPE_DF;
> +  brw_reg_type_from_bit_size(instr->def.bit_size,
> BRW_REGISTER_TYPE_D);
> fs_reg reg = bld.vgrf(reg_type, instr->def.num_components);
>
> switch (instr->def.bit_size) {
> @@ -1369,8 +1428,8 @@ fs_visitor::get_nir_src(const nir_src )
> fs_reg reg;
> if (src.is_ssa) {
>if (src.ssa->parent_instr->type == nir_instr_type_ssa_undef) {
> - const brw_reg_type reg_type = src.ssa->bit_size == 32 ?
> -BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF;
> + const brw_reg_type reg_type =
> +brw_reg_type_from_bit_size(src.ssa->bit_size,
> BRW_REGISTER_TYPE_D);
>   reg = bld.vgrf(reg_type, src.ssa->num_components);
>} else {
>   reg = nir_ssa_values[src.ssa->index];
> @@ -1404,7 +1463,7 @@ fs_visitor::get_nir_dest(const nir_dest )
>  {
> if (dest.is_ssa) {
>const brw_reg_type reg_type =
> - dest.ssa.bit_size == 32 ? BRW_REGISTER_TYPE_F :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(dest.ssa.bit_size,
> BRW_REGISTER_TYPE_F);
>nir_ssa_values[dest.ssa.index] =
>   bld.vgrf(reg_type, dest.ssa.num_components);
>return nir_ssa_values[dest.ssa.index];
> --

[Mesa-dev] [PATCH v3 12/43] i965/fs: Add brw_reg_type_from_bit_size utility method

[Jose Maria Casanova Crespo]

From: Alejandro Piñeiro 

Returns the brw_type for a given ssa.bit_size, and a reference type.
So if bit_size is 64, and the reference type is BRW_REGISTER_TYPE_F,
it returns BRW_REGISTER_TYPE_DF. The same applies if bit_size is 32
and reference type is BRW_REGISTER_TYPE_HF it returns BRW_REGISTER_TYPE_F

v2 (Jason Ekstrand):
 - Use better unreachable() messages
 - Add Q types

Signed-off-by: Jose Maria Casanova Crespo 
Signed-off-by: Alejandro Piñeiro 
---
 src/intel/compiler/brw_fs_nir.cpp | 69 ---
 1 file changed, 64 insertions(+), 5 deletions(-)

diff --git a/src/intel/compiler/brw_fs_nir.cpp 
b/src/intel/compiler/brw_fs_nir.cpp
index 7ed44f534c..affe65d5e9 100644
--- a/src/intel/compiler/brw_fs_nir.cpp
+++ b/src/intel/compiler/brw_fs_nir.cpp
@@ -227,6 +227,65 @@ fs_visitor::nir_emit_system_values()
}
 }
 
+/*
+ * Returns a type based on a reference_type (word, float, half-float) and a
+ * given bit_size.
+ *
+ * Reference BRW_REGISTER_TYPE are HF,F,DF,W,D,UW,UD.
+ *
+ * @FIXME: 64-bit return types are always DF on integer types to maintain
+ * compability with uses of DF previously to the introduction of int64
+ * support.
+ */
+static brw_reg_type
+brw_reg_type_from_bit_size(const unsigned bit_size,
+   const brw_reg_type reference_type)
+{
+   switch(reference_type) {
+   case BRW_REGISTER_TYPE_HF:
+   case BRW_REGISTER_TYPE_F:
+   case BRW_REGISTER_TYPE_DF:
+  switch(bit_size) {
+  case 16:
+ return BRW_REGISTER_TYPE_HF;
+  case 32:
+ return BRW_REGISTER_TYPE_F;
+  case 64:
+ return BRW_REGISTER_TYPE_DF;
+  default:
+ unreachable("Invalid bit size");
+  }
+   case BRW_REGISTER_TYPE_W:
+   case BRW_REGISTER_TYPE_D:
+   case BRW_REGISTER_TYPE_Q:
+  switch(bit_size) {
+  case 16:
+ return BRW_REGISTER_TYPE_W;
+  case 32:
+ return BRW_REGISTER_TYPE_D;
+  case 64:
+ return BRW_REGISTER_TYPE_DF;
+  default:
+ unreachable("Invalid bit size");
+  }
+   case BRW_REGISTER_TYPE_UW:
+   case BRW_REGISTER_TYPE_UD:
+   case BRW_REGISTER_TYPE_UQ:
+  switch(bit_size) {
+  case 16:
+ return BRW_REGISTER_TYPE_UW;
+  case 32:
+ return BRW_REGISTER_TYPE_UD;
+  case 64:
+ return BRW_REGISTER_TYPE_DF;
+  default:
+ unreachable("Invalid bit size");
+  }
+   default:
+  unreachable("Unknown type");
+   }
+}
+
 void
 fs_visitor::nir_emit_impl(nir_function_impl *impl)
 {
@@ -240,7 +299,7 @@ fs_visitor::nir_emit_impl(nir_function_impl *impl)
  reg->num_array_elems == 0 ? 1 : reg->num_array_elems;
   unsigned size = array_elems * reg->num_components;
   const brw_reg_type reg_type =
- reg->bit_size == 32 ? BRW_REGISTER_TYPE_F : BRW_REGISTER_TYPE_DF;
+ brw_reg_type_from_bit_size(reg->bit_size, BRW_REGISTER_TYPE_F);
   nir_locals[reg->index] = bld.vgrf(reg_type, size);
}
 
@@ -1341,7 +1400,7 @@ fs_visitor::nir_emit_load_const(const fs_builder ,
 nir_load_const_instr *instr)
 {
const brw_reg_type reg_type =
-  instr->def.bit_size == 32 ? BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF;
+  brw_reg_type_from_bit_size(instr->def.bit_size, BRW_REGISTER_TYPE_D);
fs_reg reg = bld.vgrf(reg_type, instr->def.num_components);
 
switch (instr->def.bit_size) {
@@ -1369,8 +1428,8 @@ fs_visitor::get_nir_src(const nir_src )
fs_reg reg;
if (src.is_ssa) {
   if (src.ssa->parent_instr->type == nir_instr_type_ssa_undef) {
- const brw_reg_type reg_type = src.ssa->bit_size == 32 ?
-BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF;
+ const brw_reg_type reg_type =
+brw_reg_type_from_bit_size(src.ssa->bit_size, BRW_REGISTER_TYPE_D);
  reg = bld.vgrf(reg_type, src.ssa->num_components);
   } else {
  reg = nir_ssa_values[src.ssa->index];
@@ -1404,7 +1463,7 @@ fs_visitor::get_nir_dest(const nir_dest )
 {
if (dest.is_ssa) {
   const brw_reg_type reg_type =
- dest.ssa.bit_size == 32 ? BRW_REGISTER_TYPE_F : BRW_REGISTER_TYPE_DF;
+ brw_reg_type_from_bit_size(dest.ssa.bit_size, BRW_REGISTER_TYPE_F);
   nir_ssa_values[dest.ssa.index] =
  bld.vgrf(reg_type, dest.ssa.num_components);
   return nir_ssa_values[dest.ssa.index];
-- 
2.13.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev