Re: [Mesa-dev] [PATCH 4/5] nir: Handle large unsigned values in opt_algebraic.

[Dylan Baker]

This seems perfectly fine to me. For what it's worth:

Reviewed-by: Dylan Baker 

Quoting Matt Turner (2016-02-04 17:48:00)
> The next patch adds an algebraic rule that uses the constant 0xff00ff00.
> 
> Without this change, the build fails with
> 
>return hex(struct.unpack('I', struct.pack('i', self.value))[0])
>struct.error: 'i' format requires -2147483648 <= number <= 2147483647
> 
> The hex() function handles integers of any size, and assigning a
> negative value to an unsigned does what we want in C. The pack/unpack is
> unnecessary (and as we see, buggy).
> ---
>  src/compiler/nir/nir_algebraic.py | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_algebraic.py 
> b/src/compiler/nir/nir_algebraic.py
> index 77ad35e..2357b57 100644
> --- a/src/compiler/nir/nir_algebraic.py
> +++ b/src/compiler/nir/nir_algebraic.py
> @@ -102,13 +102,10 @@ class Constant(Value):
>self.value = val
>  
> def __hex__(self):
> -  # Even if it's an integer, we still need to unpack as an unsigned
> -  # int.  This is because, without C99, we can only assign to the first
> -  # element of a union in an initializer.
>if isinstance(self.value, (bool)):
>   return 'NIR_TRUE' if self.value else 'NIR_FALSE'
>if isinstance(self.value, (int, long)):
> - return hex(struct.unpack('I', struct.pack('i', self.value))[0])
> + return hex(self.value)
>elif isinstance(self.value, float):
>   return hex(struct.unpack('I', struct.pack('f', self.value))[0])
>else:
> -- 
> 2.4.10
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev


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

Re: [Mesa-dev] [PATCH 4/5] nir: Handle large unsigned values in opt_algebraic.

[Kenneth Graunke]

On Thursday, February 4, 2016 5:48:00 PM PST Matt Turner wrote:
> The next patch adds an algebraic rule that uses the constant 0xff00ff00.
> 
> Without this change, the build fails with
> 
>return hex(struct.unpack('I', struct.pack('i', self.value))[0])
>struct.error: 'i' format requires -2147483648 <= number <= 2147483647
> 
> The hex() function handles integers of any size, and assigning a
> negative value to an unsigned does what we want in C. The pack/unpack is
> unnecessary (and as we see, buggy).
> ---
>  src/compiler/nir/nir_algebraic.py | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_algebraic.py
> b/src/compiler/nir/nir_algebraic.py index 77ad35e..2357b57 100644
> --- a/src/compiler/nir/nir_algebraic.py
> +++ b/src/compiler/nir/nir_algebraic.py
> @@ -102,13 +102,10 @@ class Constant(Value):
>self.value = val
> 
> def __hex__(self):
> -  # Even if it's an integer, we still need to unpack as an unsigned
> -  # int.  This is because, without C99, we can only assign to the first
> -  # element of a union in an initializer.
>if isinstance(self.value, (bool)):
>   return 'NIR_TRUE' if self.value else 'NIR_FALSE'
>if isinstance(self.value, (int, long)):
> - return hex(struct.unpack('I', struct.pack('i', self.value))[0])
> + return hex(self.value)
>elif isinstance(self.value, float):
>   return hex(struct.unpack('I', struct.pack('f', self.value))[0])
>else:

FWIW, I sent a patch to fix this on January 19th which went unreviewed:
https://lists.freedesktop.org/archives/mesa-dev/2016-January/105387.html

Your patch is probably better, though.  I never understood the point of
pack/unpacking these.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/5] nir: Handle large unsigned values in opt_algebraic.

[Kenneth Graunke]

On Monday, February 8, 2016 4:01:37 PM PST Ian Romanick wrote:
> On 02/08/2016 01:59 PM, Kenneth Graunke wrote:
> > On Thursday, February 4, 2016 5:48:00 PM PST Matt Turner wrote:
> >> The next patch adds an algebraic rule that uses the constant 0xff00ff00.
> >>
> >> Without this change, the build fails with
> >>
> >>return hex(struct.unpack('I', struct.pack('i', self.value))[0])
> >>struct.error: 'i' format requires -2147483648 <= number <= 2147483647
> >>
> >> The hex() function handles integers of any size, and assigning a
> >> negative value to an unsigned does what we want in C. The pack/unpack is
> >> unnecessary (and as we see, buggy).
> >> ---
> >>  src/compiler/nir/nir_algebraic.py | 5 +
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/src/compiler/nir/nir_algebraic.py
> >> b/src/compiler/nir/nir_algebraic.py index 77ad35e..2357b57 100644
> >> --- a/src/compiler/nir/nir_algebraic.py
> >> +++ b/src/compiler/nir/nir_algebraic.py
> >> @@ -102,13 +102,10 @@ class Constant(Value):
> >>self.value = val
> >>
> >> def __hex__(self):
> >> -  # Even if it's an integer, we still need to unpack as an unsigned
> >> -  # int.  This is because, without C99, we can only assign to the 
> >> first
> >> -  # element of a union in an initializer.
> >>if isinstance(self.value, (bool)):
> >>   return 'NIR_TRUE' if self.value else 'NIR_FALSE'
> >>if isinstance(self.value, (int, long)):
> >> - return hex(struct.unpack('I', struct.pack('i', self.value))[0])
> >> + return hex(self.value)
> >>elif isinstance(self.value, float):
> >>   return hex(struct.unpack('I', struct.pack('f', self.value))[0])
> >>else:
> > 
> > FWIW, I sent a patch to fix this on January 19th which went unreviewed:
> > https://lists.freedesktop.org/archives/mesa-dev/2016-January/105387.html
> 
> I was going to R-b it...  After you NAKed the second patch in the series
> I waited for v2.

Oh.  Sorry for the confusion.  That series was actually fine - I just
had a fabs/iabs mixup.  With that fixed, everything worked fine, and I
considered it out for review again.  I suppose I should just re-send it.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/5] nir: Handle large unsigned values in opt_algebraic.

[Ian Romanick]

On 02/08/2016 01:59 PM, Kenneth Graunke wrote:
> On Thursday, February 4, 2016 5:48:00 PM PST Matt Turner wrote:
>> The next patch adds an algebraic rule that uses the constant 0xff00ff00.
>>
>> Without this change, the build fails with
>>
>>return hex(struct.unpack('I', struct.pack('i', self.value))[0])
>>struct.error: 'i' format requires -2147483648 <= number <= 2147483647
>>
>> The hex() function handles integers of any size, and assigning a
>> negative value to an unsigned does what we want in C. The pack/unpack is
>> unnecessary (and as we see, buggy).
>> ---
>>  src/compiler/nir/nir_algebraic.py | 5 +
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir_algebraic.py
>> b/src/compiler/nir/nir_algebraic.py index 77ad35e..2357b57 100644
>> --- a/src/compiler/nir/nir_algebraic.py
>> +++ b/src/compiler/nir/nir_algebraic.py
>> @@ -102,13 +102,10 @@ class Constant(Value):
>>self.value = val
>>
>> def __hex__(self):
>> -  # Even if it's an integer, we still need to unpack as an unsigned
>> -  # int.  This is because, without C99, we can only assign to the first
>> -  # element of a union in an initializer.
>>if isinstance(self.value, (bool)):
>>   return 'NIR_TRUE' if self.value else 'NIR_FALSE'
>>if isinstance(self.value, (int, long)):
>> - return hex(struct.unpack('I', struct.pack('i', self.value))[0])
>> + return hex(self.value)
>>elif isinstance(self.value, float):
>>   return hex(struct.unpack('I', struct.pack('f', self.value))[0])
>>else:
> 
> FWIW, I sent a patch to fix this on January 19th which went unreviewed:
> https://lists.freedesktop.org/archives/mesa-dev/2016-January/105387.html

I was going to R-b it...  After you NAKed the second patch in the series
I waited for v2.

> Your patch is probably better, though.  I never understood the point of
> pack/unpacking these.
> 
> 
> 
> ___
> 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

[Mesa-dev] [PATCH 4/5] nir: Handle large unsigned values in opt_algebraic.

[Matt Turner]

The next patch adds an algebraic rule that uses the constant 0xff00ff00.

Without this change, the build fails with

   return hex(struct.unpack('I', struct.pack('i', self.value))[0])
   struct.error: 'i' format requires -2147483648 <= number <= 2147483647

The hex() function handles integers of any size, and assigning a
negative value to an unsigned does what we want in C. The pack/unpack is
unnecessary (and as we see, buggy).
---
 src/compiler/nir/nir_algebraic.py | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/compiler/nir/nir_algebraic.py 
b/src/compiler/nir/nir_algebraic.py
index 77ad35e..2357b57 100644
--- a/src/compiler/nir/nir_algebraic.py
+++ b/src/compiler/nir/nir_algebraic.py
@@ -102,13 +102,10 @@ class Constant(Value):
   self.value = val
 
def __hex__(self):
-  # Even if it's an integer, we still need to unpack as an unsigned
-  # int.  This is because, without C99, we can only assign to the first
-  # element of a union in an initializer.
   if isinstance(self.value, (bool)):
  return 'NIR_TRUE' if self.value else 'NIR_FALSE'
   if isinstance(self.value, (int, long)):
- return hex(struct.unpack('I', struct.pack('i', self.value))[0])
+ return hex(self.value)
   elif isinstance(self.value, float):
  return hex(struct.unpack('I', struct.pack('f', self.value))[0])
   else:
-- 
2.4.10

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