[Mesa-dev] [PATCH 06/59] glsl: Add a C++ code generator that uses ir_builder to rebuild a program

[Ian Romanick]

On 10/27/2016 04:16 AM, Iago Toral wrote:
> I spent some time looking at this and trying and I did not find
> anything that did not look reasonable to me in general. I have a
> question below, but in any case this is:
> 
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> 
> BTW, now I also see why you need patch 5, you want to use the
> ir_builder for things like add() directly. When I was reading that
> patch I was wondering why you would not just emit expressions with
> ir_binop_*, which I guess would have also been perfectly possible and
> would have made patch 5 unnecessary and also avoid the need for the
> if/else you have in print_without_declaration(const ir_expression *ir),
> but I guess that would've also made the generated code a bit less
> readable.

Yeah.  Ideally we'd have ir_builder functions for every operation, but
that gets hard to enforce.  I didn't want to make someone add an
ir_builder every time they add an operation.  I was trying to strike a
balance between readability of the generated code (so that you can
possibly debug it!) and future burden.

The other alternative that I considered was to generate the ir_builder
functions from Python.  That seemed like too much extra work. :)

> Iago
> 
> On Tue, 2016-10-25 at 17:59 -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
> (...)
>> +ir_visitor_status
>> +ir_builder_print_visitor::visit(ir_constant *ir)
>> +{
>> +   const unsigned my_index = next_ir_index++;
>> +
>> +   _mesa_hash_table_insert(index_map, ir, (void *)(uintptr_t)
>> my_index);
>> +
>> +   if (ir->type == glsl_type::uint_type ||
>> +       ir->type == glsl_type::int_type ||
>> +       ir->type == glsl_type::float_type ||
>> +       ir->type == glsl_type::bool_type) {
>> +      print_with_indent("ir_constant *const r%04X = ", my_index);
>> +      print_without_declaration(ir);
>> +      print_without_indent(";\n");
>> +      return visit_continue;
>> +   }
>> +
>> +   ir_constant_data all_zero;
>> +   memset(&all_zero, 0, sizeof(all_zero));
>> +
>> +   if (memcmp(&ir->value, &all_zero, sizeof(all_zero)) == 0) {
>> +      print_with_indent("ir_constant *const r%04X = ", my_index);
>> +      print_without_declaration(ir);
>> +      print_without_indent(";\n");
>> +   } else {
>> +      print_with_indent("ir_constant_data r%04X_data;\n", my_index);
>> +      print_with_indent("memset(&r%04X_data, 0,
>> sizeof(ir_constant_data));\n",
>> +                        my_index);
>> +      for (unsigned i = 0; i < 16; i++) {
>> +         switch (ir->type->base_type) {
>> +         case GLSL_TYPE_UINT:
>> +            if (ir->value.u[i] != 0)
>> +               print_without_indent("r%04X_data.u[%u] = %u;\n",
>> +                                    my_index, i, ir->value.u[i]);
>> +            break;
>> +         case GLSL_TYPE_INT:
>> +            if (ir->value.i[i] != 0)
>> +               print_without_indent("r%04X_data.i[%u] = %i;\n",
>> +                                    my_index, i, ir->value.i[i]);
>> +            break;
>> +         case GLSL_TYPE_FLOAT:
>> +            if (ir->value.u[i] != 0)
>> +               print_without_indent("r%04X_data.u[%u] = 0x%08x; /*
>> %f */\n",
>> +                                    my_index,
>> +                                    i,
>> +                                    ir->value.u[i],
>> +                                    ir->value.f[i]);
>> +            break;
>> +         case GLSL_TYPE_DOUBLE: {
>> +            uint64_t v;
>> +
>> +            STATIC_ASSERT(sizeof(double) == sizeof(uint64_t));
>> +
>> +            memcpy(&v, &ir->value.d[i], sizeof(v));
>> +            if (v != 0)
>> +               /* FIXME: This won't actually work until
>> ARB_gpu_shader_int64
>> +                * support lands.
>> +                */
>> +               print_without_indent("r%04X_data.u64[%u] = 0x%016"
>> PRIx64 "; /* %g */\n",
>> +                                    my_index, i, v, ir->value.d[i]);
> 
> Why not just copy this to data.d (instead of data.u64) as a double
> value directly?

Because of -0.  x == 0 is true when x is -0, but the bit pattern is not
0x0000000000000000.  Same goes for the GLSL_TYPE_FLOAT case above.