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

[Bryan Cain]

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.
>
> > ---
> >  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)".