[Mesa-dev] [PATCH v2 16/28] glsl/ast: Support double floats
Ilia Mirkin
imirkin at alum.mit.edu
Sat Feb 7 20:29:07 PST 2015
On Fri, Feb 6, 2015 at 3:32 AM, Ian Romanick <idr at freedesktop.org> wrote:
> Three comments near the middle / bottom...
>
> On 02/06/2015 06:56 AM, Ilia Mirkin wrote:
>> From: Dave Airlie <airlied at gmail.com>
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>> ---
>> src/glsl/ast.h | 2 ++
>> src/glsl/ast_function.cpp | 67 +++++++++++++++++++++++++++++++++--------
>> src/glsl/ast_to_hir.cpp | 38 +++++++++++++++++++++--
>> src/glsl/glsl_parser_extras.cpp | 4 +++
>> 4 files changed, 96 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
>> index cbff9d8..5401390 100644
>> --- a/src/glsl/ast_function.cpp
>> +++ b/src/glsl/ast_function.cpp
>> @@ -847,13 +876,17 @@ process_array_constructor(exec_list *instructions,
>> foreach_in_list_safe(ir_rvalue, ir, &actual_parameters) {
>> ir_rvalue *result = ir;
>>
>> + const glsl_base_type element_base_type =
>> + constructor_type->element_type()->base_type;
>> +
>> /* Apply implicit conversions (not the scalar constructor rules!). See
>> * the spec quote above. */
>> - if (constructor_type->element_type()->is_float()) {
>> - const glsl_type *desired_type =
>> - glsl_type::get_instance(GLSL_TYPE_FLOAT,
>> - ir->type->vector_elements,
>> - ir->type->matrix_columns);
>> + if (element_base_type != result->type->base_type) {
>> + const glsl_type *desired_type =
>> + glsl_type::get_instance(element_base_type,
>> + ir->type->vector_elements,
>> + ir->type->matrix_columns);
>> +
>
> It's not obvious to me why this hunk is correct. It seems like it does
> more than add support for doubles... but I'm having a little trouble
> grokking it (I'm coming down with a cold, so that isn't helping).
So... this function is dealing with
vec4[](a, b, c, d) [or dvec4/etc]
The old code used to check if the type were float... This actually
seems wrong e.g. uint can be converted to int (ivec4 vs uvec4), etc.
Here we're adding a double type, and instead of checking that the
element type is float, check that it doesn't match. If it matches, no
conversion is needed. But if the types are different (and can be
implicitly converted), we want to try to convert. Otherwise an error
is triggered a few lines down.
At least that's my read of things. I think this change is correct,
although it's not strictly related to doubles.
>
>> if (result->type->can_implicitly_convert_to(desired_type, state)) {
>> /* Even though convert_component() implements the constructor
>> * conversion rules (not the implicit conversion rules), its safe
>> @@ -1012,6 +1045,9 @@ emit_inline_vector_constructor(const glsl_type *type,
>> case GLSL_TYPE_FLOAT:
>> data.f[i + base_component] = c->get_float_component(i);
>> break;
>> + case GLSL_TYPE_DOUBLE:
>> + data.d[i + base_component] = c->get_double_component(i);
>> + break;
>> case GLSL_TYPE_BOOL:
>> data.b[i + base_component] = c->get_bool_component(i);
>> break;
>> @@ -1167,16 +1203,21 @@ emit_inline_matrix_constructor(const glsl_type *type,
>> /* Assign the scalar to the X component of a vec4, and fill the remaining
>> * components with zero.
>> */
>> + glsl_base_type param_base_type = first_param->type->base_type;
>> + assert(param_base_type == GLSL_TYPE_FLOAT ||
>> + param_base_type == GLSL_TYPE_DOUBLE);
>> ir_variable *rhs_var =
>> - new(ctx) ir_variable(glsl_type::vec4_type, "mat_ctor_vec",
>> - ir_var_temporary);
>> + new(ctx) ir_variable(glsl_type::get_instance(param_base_type, 4, 1),
>> + "mat_ctor_vec",
>> + ir_var_temporary);
>> instructions->push_tail(rhs_var);
>>
>> ir_constant_data zero;
>> - zero.f[0] = 0.0;
>> - zero.f[1] = 0.0;
>> - zero.f[2] = 0.0;
>> - zero.f[3] = 0.0;
>> + for (unsigned i = 0; i < 4; i++)
>> + if (param_base_type == GLSL_TYPE_FLOAT)
>> + zero.f[i] = 0.0;
>> + else
>> + zero.d[i] = 0.0;
>>
>> ir_instruction *inst =
>> new(ctx) ir_assignment(new(ctx) ir_dereference_variable(rhs_var),
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index ed0eb09..3ea3a13 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -172,6 +172,7 @@ get_conversion_operation(const glsl_type *to, const glsl_type *from,
>> switch (from->base_type) {
>> case GLSL_TYPE_INT: return ir_unop_i2f;
>> case GLSL_TYPE_UINT: return ir_unop_u2f;
>> + case GLSL_TYPE_DOUBLE: return ir_unop_d2f;
>> default: return (ir_expression_operation)0;
>> }
>>
>> @@ -183,6 +184,16 @@ get_conversion_operation(const glsl_type *to, const glsl_type *from,
>> default: return (ir_expression_operation)0;
>> }
>>
>> + case GLSL_TYPE_DOUBLE:
>> + if (!state->has_double())
>> + return (ir_expression_operation)0;
>> + switch (from->base_type) {
>> + case GLSL_TYPE_INT: return ir_unop_i2d;
>> + case GLSL_TYPE_UINT: return ir_unop_u2d;
>> + case GLSL_TYPE_FLOAT: return ir_unop_f2d;
>> + default: return (ir_expression_operation)0;
>> + }
>> +
>> default: return (ir_expression_operation)0;
>> }
>> }
>> @@ -340,8 +351,10 @@ arithmetic_result_type(ir_rvalue * &value_a, ir_rvalue * &value_b,
>> * type of both operands must be float.
>> */
>> assert(type_a->is_matrix() || type_b->is_matrix());
>> - assert(type_a->base_type == GLSL_TYPE_FLOAT);
>> - assert(type_b->base_type == GLSL_TYPE_FLOAT);
>> + assert(type_a->base_type == GLSL_TYPE_FLOAT ||
>> + type_a->base_type == GLSL_TYPE_DOUBLE);
>> + assert(type_b->base_type == GLSL_TYPE_FLOAT ||
>> + type_b->base_type == GLSL_TYPE_DOUBLE);
>>
>> /* "* The operator is add (+), subtract (-), or divide (/), and the
>> * operands are matrices with the same number of rows and the same
>> @@ -959,6 +972,7 @@ do_comparison(void *mem_ctx, int operation, ir_rvalue *op0, ir_rvalue *op1)
>> case GLSL_TYPE_UINT:
>> case GLSL_TYPE_INT:
>> case GLSL_TYPE_BOOL:
>> + case GLSL_TYPE_DOUBLE:
>> return new(mem_ctx) ir_expression(operation, op0, op1);
>>
>> case GLSL_TYPE_ARRAY: {
>> @@ -1746,6 +1760,10 @@ ast_expression::do_hir(exec_list *instructions,
>> result = new(ctx) ir_constant(bool(this->primary_expression.bool_constant));
>> break;
>>
>> + case ast_double_constant:
>> + result = new(ctx) ir_constant(this->primary_expression.double_constant);
>> + break;
>> +
>> case ast_sequence: {
>> /* It should not be possible to generate a sequence in the AST without
>> * any expressions in it.
>> @@ -2560,6 +2578,12 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>> _mesa_glsl_error(loc, state,
>> "varying variables may not be of type struct");
>> break;
>> + case GLSL_TYPE_DOUBLE:
>> + if (state->has_double())
>> + break;
>> + _mesa_glsl_error(loc, state,
>> + "varying variables may not be of type double");
>> + break;
>
> This block isn't necessary, is it? Doesn't the lexer generate an error
> if you use double with when has_double() is false? I think both
> GLSL_TYPE_DOUBLE and GLSL_TYPE_UINT (for the same reason) could be
> handled like GLSL_TYPE_FLOAT.
Yeah... I just noticed that it didn't generate an error for a double
constant (1.0LF), but for a 'double' token it should only work with
fp64/etc. Will just make it break here and remove the check below. I
do wonder if this can't mess something up in a future scenario with
preparsed shaders or something.
>
>> default:
>> _mesa_glsl_error(loc, state, "illegal type for a varying variable");
>> break;
>> @@ -3645,6 +3669,16 @@ ast_declarator_list::hir(exec_list *instructions,
>> var_type);
>> }
>>
>> + /* Double fragment inputs must be qualified with 'flat'. */
>> + if (state->has_double() &&
>
> Same here. The has_double() check is unnecessary. In the int case, a
> version check is necessary because the int type existed in GLSL 1.20,
> but you could not use it for some things.
>
>> + var->type->contains_double() &&
>> + var->data.interpolation != INTERP_QUALIFIER_FLAT &&
>> + state->stage == MESA_SHADER_FRAGMENT &&
>> + var->data.mode == ir_var_shader_in) {
>> + _mesa_glsl_error(&loc, state, "if a fragment input is (or contains) "
>> + "a double, then it must be qualified with 'flat'",
>> + var_type);
>> + }
>>
>> /* Interpolation qualifiers cannot be applied to 'centroid' and
>> * 'centroid varying'.
>> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
>> index cb19ce1..29e6dd2 100644
>> --- a/src/glsl/glsl_parser_extras.cpp
>> +++ b/src/glsl/glsl_parser_extras.cpp
>> @@ -962,6 +962,10 @@ ast_expression::print(void) const
>> printf("%f ", primary_expression.float_constant);
>> break;
>>
>> + case ast_double_constant:
>> + printf("%f ", primary_expression.double_constant);
>> + break;
>> +
>> case ast_bool_constant:
>> printf("%s ",
>> primary_expression.bool_constant
>>
>
More information about the mesa-dev
mailing list