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)".
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
