[Mesa-dev] [PATCH 1/3] panfrost/midgard: Set int outmod for ops writing integers

[Alyssa Rosenzweig]

By default, the "normal" output modifier is set on ALU ops. This is the
correct default for float outputs -- for floats, it preserves the semantic
value. Unfortunately, when used with integers, it does not preserve the
bitstream encoding, causing misbehaviour. (It's an open question what
happens when `normal` is used with integers -- does it apply some other
transformation? or does it do floating point normalization/etc on the
ints as if they were floats?).

Instead, we default to the "clamp to integer" output modifier for
ops writing integers. Semantically, this makes sense (clamping an
integer to the nearest integer is the identity function). In the
hardware with an integer opcode, this is the actual "normal".

This fixes numerous sporadic and sometimes bizarre bugs relating to
integers, especially integer moves. With this in place, we no longer
care about the types involved; it's just bits on the wire again.

[Jason: Once this is merged, panfrost shouldn't have any blockers to
typeless mov]

Signed-off-by: Alyssa Rosenzweig <alyssa at rosenzweig.io>
Cc: Connor Abbott <cwabbott0 at gmail.com>
Cc: Jason Ekstrand <jason.ekstrand at intel.com>
---
 .../drivers/panfrost/midgard/helpers.h        | 27 ++++++++++++++-----
 .../panfrost/midgard/midgard_compile.c        |  3 ++-
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/src/gallium/drivers/panfrost/midgard/helpers.h b/src/gallium/drivers/panfrost/midgard/helpers.h
index dc2de15931b..441c7285887 100644
--- a/src/gallium/drivers/panfrost/midgard/helpers.h
+++ b/src/gallium/drivers/panfrost/midgard/helpers.h
@@ -71,6 +71,9 @@
 /* Is the op commutative? */
 #define OP_COMMUTES (1 << 3)
 
+/* Does the op convert types between int- and float- space (i2f/f2u/etc) */
+#define OP_TYPE_CONVERT (1 << 4)
+
 /* Vector-independant shorthands for the above; these numbers are arbitrary and
  * not from the ISA. Convert to the above with unit_enum_to_midgard */
 
@@ -207,11 +210,11 @@ static struct {
         [midgard_alu_op_fexp2]		 = {"fexp2", UNIT_VLUT},
         [midgard_alu_op_flog2]		 = {"flog2", UNIT_VLUT},
 
-        [midgard_alu_op_f2i]		 = {"f2i", UNITS_ADD},
-        [midgard_alu_op_f2u]		 = {"f2u", UNITS_ADD},
-        [midgard_alu_op_f2u8]		 = {"f2u8", UNITS_ADD},
-        [midgard_alu_op_i2f]		 = {"i2f", UNITS_ADD},
-        [midgard_alu_op_u2f]		 = {"u2f", UNITS_ADD},
+        [midgard_alu_op_f2i]		 = {"f2i", UNITS_ADD | OP_TYPE_CONVERT},
+        [midgard_alu_op_f2u]		 = {"f2u", UNITS_ADD | OP_TYPE_CONVERT},
+        [midgard_alu_op_f2u8]		 = {"f2u8", UNITS_ADD | OP_TYPE_CONVERT},
+        [midgard_alu_op_i2f]		 = {"i2f", UNITS_ADD | OP_TYPE_CONVERT},
+        [midgard_alu_op_u2f]		 = {"u2f", UNITS_ADD | OP_TYPE_CONVERT},
 
         [midgard_alu_op_fsin]		 = {"fsin", UNIT_VLUT},
         [midgard_alu_op_fcos]		 = {"fcos", UNIT_VLUT},
@@ -262,7 +265,7 @@ static struct {
 /* Is this opcode that of an integer (regardless of signedness)? Instruction
  * names authoritatively determine types */
 
-static bool
+static inline bool
 midgard_is_integer_op(int op)
 {
         const char *name = alu_opcode_props[op].name;
@@ -272,3 +275,15 @@ midgard_is_integer_op(int op)
 
         return (name[0] == 'i') || (name[0] == 'u');
 }
+
+/* Does this opcode *write* an integer? Same as is_integer_op, unless it's a
+ * conversion between int<->float in which case we do the opposite */
+
+static inline bool
+midgard_is_integer_out_op(int op)
+{
+        bool is_int = midgard_is_integer_op(op);
+        bool is_conversion = alu_opcode_props[op].props & OP_TYPE_CONVERT;
+
+        return is_int ^ is_conversion;
+}
diff --git a/src/gallium/drivers/panfrost/midgard/midgard_compile.c b/src/gallium/drivers/panfrost/midgard/midgard_compile.c
index 4a26ba769b2..496ecb02e09 100644
--- a/src/gallium/drivers/panfrost/midgard/midgard_compile.c
+++ b/src/gallium/drivers/panfrost/midgard/midgard_compile.c
@@ -1336,8 +1336,9 @@ emit_alu(compiler_context *ctx, nir_alu_instr *instr)
                 return;
         }
 
-        /* Midgard can perform certain modifiers on output ofa n ALU op */
+        /* 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;
 
         /* fmax(a, 0.0) can turn into a .pos modifier as an optimization */
-- 
2.20.1