From 6780481a3f49f09692017a926f2b49e38dd169bc Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Wed, 5 Jun 2019 15:17:45 -0700 Subject: [PATCH] panfrost/midgard: Add integer outmods For floats, output modifiers determine clamping behaviour. For integers, they determine wrapping/saturation behaviour (or shifting -- see next commit). These are very different; they are conceptually two unrelated enums union'ed together; the distinction is responsible for many-a-bug. While clamping behaviour for floats was clear from GL, the int behaviour is only known From OpenCL contortion with convert_*_sat() functions. With the underlying functions known, clean up the codebase, likely fixing outmod type related bugs in the process. Signed-off-by: Alyssa Rosenzweig --- .../drivers/panfrost/midgard/disassemble.c | 22 ++++++--- .../drivers/panfrost/midgard/midgard.h | 17 +++++-- .../panfrost/midgard/midgard_compile.c | 46 +++++++++++++------ 3 files changed, 60 insertions(+), 25 deletions(-) diff --git a/src/gallium/drivers/panfrost/midgard/disassemble.c b/src/gallium/drivers/panfrost/midgard/disassemble.c index a2a0b0dba37..bf66d1a7da7 100644 --- a/src/gallium/drivers/panfrost/midgard/disassemble.c +++ b/src/gallium/drivers/panfrost/midgard/disassemble.c @@ -132,17 +132,25 @@ print_reg(unsigned reg, unsigned bits) printf("r%u", reg); } -static char *outmod_names[4] = { +static char *outmod_names_float[4] = { "", ".pos", - ".int", + ".unk2", ".sat" }; +static char *outmod_names_int[4] = { + ".isat", + ".usat", + "", + ".unk3" +}; + static void -print_outmod(midgard_outmod outmod) +print_outmod(unsigned outmod, bool is_int) { - printf("%s", outmod_names[outmod]); + printf("%s", is_int ? outmod_names_int[outmod] : + outmod_names_float[outmod]); } static void @@ -377,7 +385,8 @@ print_vector_field(const char *name, uint16_t *words, uint16_t reg_word, printf("%s.", name); print_alu_opcode(alu_field->op); - print_outmod(alu_field->outmod); + print_outmod(alu_field->outmod, + midgard_is_integer_out_op(alu_field->op)); printf(" "); bool out_high = false; @@ -503,7 +512,8 @@ print_scalar_field(const char *name, uint16_t *words, uint16_t reg_word, printf("%s.", name); print_alu_opcode(alu_field->op); - print_outmod(alu_field->outmod); + print_outmod(alu_field->outmod, + midgard_is_integer_out_op(alu_field->op)); printf(" "); if (alu_field->output_full) diff --git a/src/gallium/drivers/panfrost/midgard/midgard.h b/src/gallium/drivers/panfrost/midgard/midgard.h index 7bf17321ccb..ff853a19733 100644 --- a/src/gallium/drivers/panfrost/midgard/midgard.h +++ b/src/gallium/drivers/panfrost/midgard/midgard.h @@ -172,15 +172,22 @@ typedef enum { typedef enum { midgard_outmod_none = 0, midgard_outmod_pos = 1, - midgard_outmod_int = 2, + /* 0x2 unknown */ midgard_outmod_sat = 3 -} midgard_outmod; +} midgard_outmod_float; + +typedef enum { + midgard_outmod_int_saturate = 0, + midgard_outmod_uint_saturate = 1, + midgard_outmod_int_wrap = 2, + /* 0x3 unknown */ +} midgard_outmod_int; typedef enum { midgard_reg_mode_8 = 0, midgard_reg_mode_16 = 1, midgard_reg_mode_32 = 2, - midgard_reg_mode_64 = 3 /* TODO: verify */ + midgard_reg_mode_64 = 3 } midgard_reg_mode; typedef enum { @@ -224,7 +231,7 @@ __attribute__((__packed__)) unsigned src1 : 13; unsigned src2 : 13; midgard_dest_override dest_override : 2; - midgard_outmod outmod : 2; + midgard_outmod_float outmod : 2; unsigned mask : 8; } midgard_vector_alu; @@ -246,7 +253,7 @@ __attribute__((__packed__)) unsigned src1 : 6; unsigned src2 : 11; unsigned unknown : 1; - midgard_outmod outmod : 2; + unsigned outmod : 2; bool output_full : 1; unsigned output_component : 3; } diff --git a/src/gallium/drivers/panfrost/midgard/midgard_compile.c b/src/gallium/drivers/panfrost/midgard/midgard_compile.c index af6f26855b0..bc9521aa9ed 100644 --- a/src/gallium/drivers/panfrost/midgard/midgard_compile.c +++ b/src/gallium/drivers/panfrost/midgard/midgard_compile.c @@ -540,14 +540,14 @@ emit_condition(compiler_context *ctx, nir_src *src, bool for_branch, unsigned co .unit = for_branch ? UNIT_SMUL : UNIT_SADD, .ssa_args = { - .src0 = condition, .src1 = condition, .dest = SSA_FIXED_REGISTER(31), }, + .alu = { .op = midgard_alu_op_iand, - .outmod = midgard_outmod_int, + .outmod = midgard_outmod_int_wrap, .reg_mode = midgard_reg_mode_32, .dest_override = midgard_dest_override_none, .mask = (0x3 << 6), /* w */ @@ -586,7 +586,7 @@ emit_condition_mixed(compiler_context *ctx, nir_alu_src *src, unsigned nr_comp) }, .alu = { .op = midgard_alu_op_iand, - .outmod = midgard_outmod_int, + .outmod = midgard_outmod_int_wrap, .reg_mode = midgard_reg_mode_32, .dest_override = midgard_dest_override_none, .mask = expand_writemask((1 << nr_comp) - 1), @@ -617,7 +617,7 @@ emit_indirect_offset(compiler_context *ctx, nir_src *src) }, .alu = { .op = midgard_alu_op_imov, - .outmod = midgard_outmod_int, + .outmod = midgard_outmod_int_wrap, .reg_mode = midgard_reg_mode_32, .dest_override = midgard_dest_override_none, .mask = (0x3 << 6), /* w */ @@ -821,12 +821,14 @@ emit_alu(compiler_context *ctx, nir_alu_instr *instr) } /* Midgard can perform certain modifiers on output of an ALU op */ - midgard_outmod outmod = - midgard_is_integer_out_op(op) ? midgard_outmod_int : - instr->dest.saturate ? midgard_outmod_sat : midgard_outmod_none; + unsigned outmod; - if (instr->op == nir_op_fsat) - outmod = midgard_outmod_sat; + if (midgard_is_integer_out_op(op)) { + outmod = midgard_outmod_int_wrap; + } else { + bool sat = instr->dest.saturate || instr->op == nir_op_fsat; + outmod = sat ? midgard_outmod_sat : midgard_outmod_none; + } /* fmax(a, 0.0) can turn into a .pos modifier as an optimization */ @@ -1765,6 +1767,18 @@ mir_nontrivial_source2_mod(midgard_instruction *ins) return mir_nontrivial_mod(src2, is_int, mask); } +static bool +mir_nontrivial_outmod(midgard_instruction *ins) +{ + bool is_int = midgard_is_integer_op(ins->alu.op); + unsigned mod = ins->alu.outmod; + + if (is_int) + return mod != midgard_outmod_int_wrap; + else + return mod != midgard_outmod_none; +} + static bool midgard_opt_copy_prop(compiler_context *ctx, midgard_block *block) { @@ -1789,7 +1803,7 @@ midgard_opt_copy_prop(compiler_context *ctx, midgard_block *block) if (ins->has_constants) continue; if (mir_nontrivial_source2_mod(ins)) continue; - if (ins->alu.outmod != midgard_outmod_none) continue; + if (mir_nontrivial_outmod(ins)) continue; /* We're clear -- rewrite */ mir_rewrite_index_src(ctx, to, from); @@ -1804,7 +1818,7 @@ midgard_opt_copy_prop(compiler_context *ctx, midgard_block *block) * the move can be propagated away entirely */ static bool -mir_compose_outmod(midgard_outmod *outmod, midgard_outmod comp) +mir_compose_float_outmod(midgard_outmod_float *outmod, midgard_outmod_float comp) { /* Nothing to do */ if (comp == midgard_outmod_none) @@ -1832,6 +1846,7 @@ midgard_opt_pos_propagate(compiler_context *ctx, midgard_block *block) /* TODO: Registers? */ unsigned src = ins->ssa_args.src1; if (src >= ctx->func->impl->ssa_alloc) continue; + assert(!mir_has_multiple_writes(ctx, src)); /* There might be a source modifier, too */ if (mir_nontrivial_source2_mod(ins)) continue; @@ -1841,8 +1856,11 @@ midgard_opt_pos_propagate(compiler_context *ctx, midgard_block *block) if (v->type != TAG_ALU_4) continue; if (v->ssa_args.dest != src) continue; - midgard_outmod temp = v->alu.outmod; - progress |= mir_compose_outmod(&temp, ins->alu.outmod); + /* Can we even take a float outmod? */ + if (midgard_is_integer_out_op(v->alu.op)) continue; + + midgard_outmod_float temp = v->alu.outmod; + progress |= mir_compose_float_outmod(&temp, ins->alu.outmod); /* Throw in the towel.. */ if (!progress) break; @@ -2076,7 +2094,7 @@ emit_blend_epilogue(compiler_context *ctx) .op = midgard_alu_op_imov, .reg_mode = midgard_reg_mode_8, .dest_override = midgard_dest_override_none, - .outmod = midgard_outmod_int, + .outmod = midgard_outmod_int_wrap, .mask = 0xFF, .src1 = vector_alu_srco_unsigned(blank_alu_src), .src2 = vector_alu_srco_unsigned(blank_alu_src), -- 2.30.2