[Mesa-dev] [PATCH 1/3] glsl: correctly handle conversions between int and uint

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/18/2011 04:51 PM, Bryan Cain wrote:
> This patch was revised several times before submitting, and even after
> that I still wasn't sure that adding conversion operations was the right
> solution.  So I'm not really surprised that there are problems with this
> one. :)
> 
> On 04/18/2011 06:36 PM, Ian Romanick wrote:
>> On 04/17/2011 11:39 PM, Bryan Cain wrote:
>>> From the GLSL 1.30 specification, section 4.1.10 ("Implicit
>> Conversions"):
>>>     "There are no implicit conversions between signed and unsigned
>> integers."
>>
>>> However, convert_component() was assuming that conversions between
>> int and uint were implicit.
>>
>> The conversions applied in convert_component only apply to constructor
>> parameters.  As Ken mentions, in those cases we want
>>
>> Internally we treat signed and unsigned identically because the
>> representation is the same.  There's no way for an application to
>> observe the difference between:
>>
>>     ivec4 a = ivec4(5 , 6 , 7 , 8 );
>>     ivec4 b = ivec4(5 , 6 , 7 , 8u);
>>     ivec4 c = ivec4(5 , 6 , 7u, 8 );
>>     ivec4 d = ivec4(5 , 6 , 7u, 8u);
>>     ivec4 e = ivec4(5 , 6u, 7 , 8 );
>>     ivec4 f = ivec4(5 , 6u, 7 , 8u);
>>     ivec4 g = ivec4(5 , 6u, 7u, 8 );
>>     ivec4 h = ivec4(5 , 6u, 7u, 8u);
>>     // etc.
>>
>> The type checker ensures that operands to operations that could exhibit
>> differing behavior (e.g., ir_bin_mul) have identical types.  That code
>> lives in arithmetic_result_type in ast_to_hir.cpp.  There is similar
>> code in match_function_by_name in ast_function.cpp.
>>
>> Do you have any specific test cases in mind that don't produce correct
>> results with the existing code?
> Yes, usages like this:
> 
> int var1 = 7;
> uint var2 = uint(var1);
> 
> They were causing a type mismatch error in ast_to_hir.cpp because
> convert_component() was treating the uint(int) constructor as a no-op.

Ah, that makes sense.  I just sent some patches to the piglit mailing
list with some tests in this vein.

You've convinced me that we need i2u and u2i unary operators.  I don't
think we need b2u and u2b operators because we can just do (i2b(u2i
value)) and (i2u(b2i value)).  Ditto for f2i.  This should result in
less churn in the backends since u2i and i2u will universally be no-ops.

Almost every place that checks is_integer() will need to check for the
specific type, either GLSL_TYPE_UINT or GLSL_TYPE_INT, that it expects.
 This means that your patch 2/3, which is already applied, will need to
be reverted.

>>> ---
>>>  src/glsl/ast_function.cpp           |   16 ++++++++++++----
>>>  src/glsl/ir.cpp                     |    8 ++++++++
>>>  src/glsl/ir.h                       |    2 ++
>>>  src/glsl/ir_constant_expression.cpp |    8 ++++++++
>>>  src/glsl/ir_validate.cpp            |    8 ++++++++
>>>  5 files changed, 38 insertions(+), 4 deletions(-)
>>
>>> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
>>> index e5cb873..cc3f032 100644
>>> --- a/src/glsl/ast_function.cpp
>>> +++ b/src/glsl/ast_function.cpp
>>> @@ -267,17 +267,25 @@ convert_component(ir_rvalue *src, const
>> glsl_type *desired_type)
>>>     assert(a <= GLSL_TYPE_BOOL);
>>>     assert(b <= GLSL_TYPE_BOOL);
>>
>>> -   if ((a == b) || (src->type->is_integer() &&
>> desired_type->is_integer()))
>>> +   if (a == b)
>>>        return src;
>>
>>>     switch (a) {
>>>     case GLSL_TYPE_UINT:
>>>     case GLSL_TYPE_INT:
>>> -      if (b == GLSL_TYPE_FLOAT)
>>> +      switch(b) {
>>> +      case GLSL_TYPE_UINT:
>>> +     result = new(ctx) ir_expression(ir_unop_u2i, desired_type,
>> src, NULL);
>>> +     break;
>>> +      case GLSL_TYPE_INT:
>>> +     result = new(ctx) ir_expression(ir_unop_i2u, desired_type,
>> src, NULL);
>>> +     break;
>>> +      case GLSL_TYPE_FLOAT:
>>>       result = new(ctx) ir_expression(ir_unop_f2i, desired_type,
>> src, NULL);
>>
>> With the other changes, leaving this as-is is kind of hinkey.  Here the
>> result base type will be GLSL_TYPE_INT, but the desired based type could
>> be GLSL_TYPE_UINT.
> Unless I'm missing something, that case is handled by the line "if (a ==
> b)".

What I meant was uint(float) and int(float) will both generate an
ir_unop_f2i, which has type int.  So, 'uint x = uint(5.2);' will result
in a type error.  Right?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2uEBkACgkQX1gOwKyEAw8esQCdEawL5QiLogZuqLrstfyI7Dz9
ub0AoJuTNzMseA2K3vdrtSzjEQ8sLctO
=yFr3
-----END PGP SIGNATURE-----