[Mesa-dev] [PATCH 3/6] panfrost/midgard: Add integer outmods

Alyssa Rosenzweig alyssa.rosenzweig at collabora.com
Wed Jun 5 22:46:50 UTC 2019


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 <alyssa.rosenzweig at collabora.com>
---
 .../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 c467e94fc29..1a25d0565f4 100644
--- a/src/gallium/drivers/panfrost/midgard/disassemble.c
+++ b/src/gallium/drivers/panfrost/midgard/disassemble.c
@@ -107,17 +107,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
@@ -352,7 +360,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;
@@ -478,7 +487,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 c25f10c4028..b4eb788f813 100644
--- a/src/gallium/drivers/panfrost/midgard/midgard.h
+++ b/src/gallium/drivers/panfrost/midgard/midgard.h
@@ -187,15 +187,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 {
@@ -239,7 +246,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;
@@ -261,7 +268,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 a0e4d05fd05..585c9fa7a75 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 */
@@ -820,12 +820,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 */
 
@@ -1753,6 +1755,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)
 {
@@ -1777,7 +1791,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);
@@ -1792,7 +1806,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)
@@ -1820,6 +1834,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;
@@ -1829,8 +1844,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;
@@ -2098,7 +2116,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.20.1



More information about the mesa-dev mailing list