On 23 August 2014 10:43, Ian Romanick <[email protected]> wrote:
> There is a handful of minor comments below.
>
> There is one additional thing missing: handling doubles in UBOs. Many
> places in the linker (a bunch of which I just changed) assume, for
> alignment purposes, that every basic type is 4-bytes. Several places in
> the std140 packing rules say things like "...rounded up to the base
> alignment of a vec4." In most of those places we just set the alignment
> to 16 because it will be. :) With dvec4, the alignment could be 32 instead.
>
> I think we can punt on that for a little bit. I'm putting together a
> random UBO test generator that should hit "all" the cases. I hope to
> have that out on the piglit list next week.
Okay I'll await that,
>> new(ctx) ir_expression(ir_unop_b2i, src));
>> break;
>> + case GLSL_TYPE_DOUBLE:
>> + result = new(ctx) ir_expression(ir_unop_f2u,
>> + new(ctx) ir_expression(ir_unop_d2f, src));
>> + break;
>> }
>> break;
>> case GLSL_TYPE_INT:
>> @@ -583,6 +587,10 @@ convert_component(ir_rvalue *src, const glsl_type
>> *desired_type)
>> case GLSL_TYPE_BOOL:
>> result = new(ctx) ir_expression(ir_unop_b2i, src);
>> break;
>> + case GLSL_TYPE_DOUBLE:
>> + result = new(ctx) ir_expression(ir_unop_f2i,
>> + new(ctx) ir_expression(ir_unop_d2f, src));
>
> Don't we just want an ir_unop_d2i? There are values that can be exactly
> represented in a double and a 32-bit integer that cannot be represented
> in a float... right?
Well I wasn't sure, the hw doesn't seem to have one at least on radeon,
however Tapani has added i2d and u2d for implicit conversions in his branch,
so I think I should just add d2i/d2u/i2d/u2d to the IR and I can hide it in
gallium until we see hw that actually does this properly, radeon on evergreen
does it via floats.
>> break;
>> + case GLSL_TYPE_DOUBLE:
>> + if (state->is_version(400, 0) || state->ARB_gpu_shader_fp64_enable)
>
> Maybe add a has_double (like has_explicit_attrib_stream) predicate? I
> bet this same check happens elsewhere...
Yup sounds good done this.
>> + if ((state->is_version(400, 0) || state->ARB_gpu_shader_fp64_enable)
>> &&
>> + var->type->contains_double() &&
>> + var->data.interpolation != INTERP_QUALIFIER_FLAT &&
>> + ((state->stage == MESA_SHADER_FRAGMENT && var->data.mode ==
>> ir_var_shader_in)
>> + )) {
>> + const char *var_type = (state->stage == MESA_SHADER_VERTEX) ?
>> + "vertex output" : "fragment input";
>> + _mesa_glsl_error(&loc, state, "if a %s is (or contains) "
>> + "a double, then it must be qualified with 'flat'",
>> + var_type);
>
> Is anything needed for geometry shaders here?
hmm not sure, will take a look.
>> +double
>> +ir_constant::get_double_component(unsigned i) const
>> +{
>> + switch (this->type->base_type) {
>> + case GLSL_TYPE_UINT: return (double) this->value.u[i];
>> + case GLSL_TYPE_INT: return (double) this->value.i[i];
>> + case GLSL_TYPE_FLOAT: return (double) this->value.f[i];
>> + case GLSL_TYPE_BOOL: return this->value.b[i] ? 1.0 : 0.0;
>> + case GLSL_TYPE_DOUBLE: return this->value.d[i];
>> + default: assert(!"Should not get here."); break;
>
> Use unreachable("Invalid base type") here...
We don't do this for any of these cases, so it should either be a separate
cleanup patch to fix it in all cases, or a pre-patch to fix all the current ones
first?
Thanks for the review,
I'll update my patches and pull some of Tapani's work in as well
before reposting.
Dave.
_______________________________________________
mesa-dev mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
