[Mesa-dev] [PATCH 07/11] glsl: add double support

[Dave Airlie]

On 23 August 2014 10:43, Ian Romanick <idr at freedesktop.org> 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.