Re: [Mesa-dev] [PATCH v2 1/2] spirv: fix OpSConvert when the source is unsigned

[Samuel Iglesias Gonsálvez]

On 12/03/18 20:42, Jason Ekstrand wrote:
> On Mon, Mar 5, 2018 at 10:21 PM, Samuel Iglesias Gonsálvez
> > wrote:
>
> OpSConvert interprets the MSB of the unsigned value as the sign
> bit and
> extends it to the new type. If we want to preserve the value, we need
> to use OpUConvert opcode.
>
> v2:
> - No need to check dst type.
> - Fix typo in comment.
>
> Signed-off-by: Samuel Iglesias Gonsálvez  >
> ---
>  src/compiler/spirv/vtn_alu.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/compiler/spirv/vtn_alu.c
> b/src/compiler/spirv/vtn_alu.c
> index d0c9e316935..a5cefc35773 100644
> --- a/src/compiler/spirv/vtn_alu.c
> +++ b/src/compiler/spirv/vtn_alu.c
> @@ -354,10 +354,26 @@ vtn_nir_alu_op_for_spirv_opcode(struct
> vtn_builder *b,
>     case SpvOpConvertFToS:
>     case SpvOpConvertSToF:
>     case SpvOpConvertUToF:
> -   case SpvOpSConvert:
>     case SpvOpFConvert:
>        return nir_type_conversion_op(src, dst,
> nir_rounding_mode_undef);
>
> +   case SpvOpSConvert: {
> +      nir_alu_type src_base = (nir_alu_type)
> nir_alu_type_get_base_type(src);
> +      if (src_base == nir_type_uint) {
>
>
> Why are we predicating this on src_base == nir_type_uint?  It seems to
> me as if we should just ignore the source and destination type except
> for the bit size.

Right.

>  
>
> +         /* SPIR-V expects to interpret the unsigned value as
> signed and
> +          * do sign extend. Return the opcode accordingly.
> +          */
> +         unsigned dst_bit_size = nir_alu_type_get_type_size(dst);
> +         switch (dst_bit_size) {
> +         case 16:   return nir_op_i2i16;
> +         case 32:   return nir_op_i2i32;
> +         case 64:   return nir_op_i2i64;
>
>
> This can be nir_type_int | dst_bit_size. NIR types are convenient like
> that. :-)

Oh, nice suggestion! I am going to send soon a v3 with the suggested
changes for both patches.

Thanks a lot!

Sam

>  
>
> +         default:
> +            vtn_fail("Invalid nir alu bit size");
> +         }
> +      }
> +      return nir_type_conversion_op(src, dst,
> nir_rounding_mode_undef);
> +   }
>     /* Derivatives: */
>     case SpvOpDPdx:         return nir_op_fddx;
>     case SpvOpDPdy:         return nir_op_fddy;
> --
> 2.14.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org 
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>
>



signature.asc
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] spirv: fix OpSConvert when the source is unsigned

[Jason Ekstrand]

On Mon, Mar 5, 2018 at 10:21 PM, Samuel Iglesias Gonsálvez <
sigles...@igalia.com> wrote:

> OpSConvert interprets the MSB of the unsigned value as the sign bit and
> extends it to the new type. If we want to preserve the value, we need
> to use OpUConvert opcode.
>
> v2:
> - No need to check dst type.
> - Fix typo in comment.
>
> Signed-off-by: Samuel Iglesias Gonsálvez 
> ---
>  src/compiler/spirv/vtn_alu.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
> index d0c9e316935..a5cefc35773 100644
> --- a/src/compiler/spirv/vtn_alu.c
> +++ b/src/compiler/spirv/vtn_alu.c
> @@ -354,10 +354,26 @@ vtn_nir_alu_op_for_spirv_opcode(struct vtn_builder
> *b,
> case SpvOpConvertFToS:
> case SpvOpConvertSToF:
> case SpvOpConvertUToF:
> -   case SpvOpSConvert:
> case SpvOpFConvert:
>return nir_type_conversion_op(src, dst, nir_rounding_mode_undef);
>
> +   case SpvOpSConvert: {
> +  nir_alu_type src_base = (nir_alu_type) nir_alu_type_get_base_type(
> src);
> +  if (src_base == nir_type_uint) {
>

Why are we predicating this on src_base == nir_type_uint?  It seems to me
as if we should just ignore the source and destination type except for the
bit size.


> + /* SPIR-V expects to interpret the unsigned value as signed and
> +  * do sign extend. Return the opcode accordingly.
> +  */
> + unsigned dst_bit_size = nir_alu_type_get_type_size(dst);
> + switch (dst_bit_size) {
> + case 16:   return nir_op_i2i16;
> + case 32:   return nir_op_i2i32;
> + case 64:   return nir_op_i2i64;
>

This can be nir_type_int | dst_bit_size. NIR types are convenient like
that. :-)


> + default:
> +vtn_fail("Invalid nir alu bit size");
> + }
> +  }
> +  return nir_type_conversion_op(src, dst, nir_rounding_mode_undef);
> +   }
> /* Derivatives: */
> case SpvOpDPdx: return nir_op_fddx;
> case SpvOpDPdy: return nir_op_fddy;
> --
> 2.14.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

[Mesa-dev] [PATCH v2 1/2] spirv: fix OpSConvert when the source is unsigned

[Samuel Iglesias Gonsálvez]

OpSConvert interprets the MSB of the unsigned value as the sign bit and
extends it to the new type. If we want to preserve the value, we need
to use OpUConvert opcode.

v2:
- No need to check dst type.
- Fix typo in comment.

Signed-off-by: Samuel Iglesias Gonsálvez 
---
 src/compiler/spirv/vtn_alu.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
index d0c9e316935..a5cefc35773 100644
--- a/src/compiler/spirv/vtn_alu.c
+++ b/src/compiler/spirv/vtn_alu.c
@@ -354,10 +354,26 @@ vtn_nir_alu_op_for_spirv_opcode(struct vtn_builder *b,
case SpvOpConvertFToS:
case SpvOpConvertSToF:
case SpvOpConvertUToF:
-   case SpvOpSConvert:
case SpvOpFConvert:
   return nir_type_conversion_op(src, dst, nir_rounding_mode_undef);
 
+   case SpvOpSConvert: {
+  nir_alu_type src_base = (nir_alu_type) nir_alu_type_get_base_type(src);
+  if (src_base == nir_type_uint) {
+ /* SPIR-V expects to interpret the unsigned value as signed and
+  * do sign extend. Return the opcode accordingly.
+  */
+ unsigned dst_bit_size = nir_alu_type_get_type_size(dst);
+ switch (dst_bit_size) {
+ case 16:   return nir_op_i2i16;
+ case 32:   return nir_op_i2i32;
+ case 64:   return nir_op_i2i64;
+ default:
+vtn_fail("Invalid nir alu bit size");
+ }
+  }
+  return nir_type_conversion_op(src, dst, nir_rounding_mode_undef);
+   }
/* Derivatives: */
case SpvOpDPdx: return nir_op_fddx;
case SpvOpDPdy: return nir_op_fddy;
-- 
2.14.1

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