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

[Bryan Cain]

On 04/19/2011 05:43 PM, Ian Romanick wrote:
> 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.

Wouldn't it only apply to checks for destination types of most
operations, not source types?

In fact, if instructions store the destination type, what cases would
require the compiler beyond ast_to_hir to even care about the difference
between int and uint?

>
> >>> ---
> >>>  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?

It's not resulting in one for me with the patch applied.  Are the exact
source and destination types not stored as part of an expression in GLSL IR?