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 <ito...@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.roman...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
