intel/compiler: validate conversions between 64-bit and 8-bit types

v2:
 - Add some tests with UB type too (Jason)

v3:
 - consider implicit conversions from 2src instructions too (Curro).

v4:
 - Do not check src1 type in single-source instructions (Curro).

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> (v2)

diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c
index 54bffb3af0367c97320e555b0aebc65fbf76b6c6..cfaf126e2f5f736fbc7ccf4ec29b965792fb7342 100644 (file)
--- a/src/intel/compiler/brw_eu_validate.c
+++ b/src/intel/compiler/brw_eu_validate.c
@@ -529,6 +529,31 @@ is_mixed_float(const struct gen_device_info *devinfo, const brw_inst *inst)
           types_are_mixed_float(src1_type, dst_type);
 }
 
+/**
+ * Returns whether an instruction is an explicit or implicit conversion
+ * to/from byte.
+ */
+static bool
+is_byte_conversion(const struct gen_device_info *devinfo,
+                   const brw_inst *inst)
+{
+   enum brw_reg_type dst_type = brw_inst_dst_type(devinfo, inst);
+
+   unsigned num_sources = num_sources_from_inst(devinfo, inst);
+   enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
+
+   if (dst_type != src0_type &&
+       (type_sz(dst_type) == 1 || type_sz(src0_type) == 1)) {
+      return true;
+   } else if (num_sources > 1) {
+      enum brw_reg_type src1_type = brw_inst_src1_type(devinfo, inst);
+      return dst_type != src1_type &&
+            (type_sz(dst_type) == 1 || type_sz(src1_type) == 1);
+   }
+
+   return false;
+}
+
 /**
  * Checks restrictions listed in "General Restrictions Based on Operand Types"
  * in the "Register Region Restrictions" section.
@@ -599,6 +624,31 @@ general_restrictions_based_on_operand_types(const struct gen_device_info *devinf
        exec_type_size == 8 && dst_type_size == 4)
       dst_type_size = 8;
 
+   if (is_byte_conversion(devinfo, inst)) {
+      /* From the BDW+ PRM, Volume 2a, Command Reference, Instructions - MOV:
+       *
+       *    "There is no direct conversion from B/UB to DF or DF to B/UB.
+       *     There is no direct conversion from B/UB to Q/UQ or Q/UQ to B/UB."
+       *
+       * Even if these restrictions are listed for the MOV instruction, we
+       * validate this more generally, since there is the possibility
+       * of implicit conversions from other instructions.
+       */
+      enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
+      enum brw_reg_type src1_type = num_sources > 1 ?
+                                    brw_inst_src1_type(devinfo, inst) : 0;
+
+      ERROR_IF(type_sz(dst_type) == 1 &&
+               (type_sz(src0_type) == 8 ||
+                (num_sources > 1 && type_sz(src1_type) == 8)),
+               "There are no direct conversions between 64-bit types and B/UB");
+
+      ERROR_IF(type_sz(dst_type) == 8 &&
+               (type_sz(src0_type) == 1 ||
+                (num_sources > 1 && type_sz(src1_type) == 1)),
+               "There are no direct conversions between 64-bit types and B/UB");
+   }
+
    if (is_half_float_conversion(devinfo, inst)) {
       /**
        * A helper to validate used in the validation of the following restriction

diff --git a/src/intel/compiler/test_eu_validate.cpp b/src/intel/compiler/test_eu_validate.cpp
index 3fdbecb003b62809631c98dc460385f8cf7b7397..2e06da2f5b4a170b91fd3e931890ec05678e6c2c 100644 (file)
--- a/src/intel/compiler/test_eu_validate.cpp
+++ b/src/intel/compiler/test_eu_validate.cpp
@@ -848,6 +848,61 @@ TEST_P(validation_test, byte_destination_relaxed_alignment)
    }
 }
 
+TEST_P(validation_test, byte_64bit_conversion)
+{
+   static const struct {
+      enum brw_reg_type dst_type;
+      enum brw_reg_type src_type;
+      unsigned dst_stride;
+      bool expected_result;
+   } inst[] = {
+#define INST(dst_type, src_type, dst_stride, expected_result)             \
+      {                                                                   \
+         BRW_REGISTER_TYPE_##dst_type,                                    \
+         BRW_REGISTER_TYPE_##src_type,                                    \
+         BRW_HORIZONTAL_STRIDE_##dst_stride,                              \
+         expected_result,                                                 \
+      }
+
+      INST(B,   Q, 1, false),
+      INST(B,  UQ, 1, false),
+      INST(B,  DF, 1, false),
+      INST(UB,  Q, 1, false),
+      INST(UB, UQ, 1, false),
+      INST(UB, DF, 1, false),
+
+      INST(B,   Q, 2, false),
+      INST(B,  UQ, 2, false),
+      INST(B , DF, 2, false),
+      INST(UB,  Q, 2, false),
+      INST(UB, UQ, 2, false),
+      INST(UB, DF, 2, false),
+
+      INST(B,   Q, 4, false),
+      INST(B,  UQ, 4, false),
+      INST(B,  DF, 4, false),
+      INST(UB,  Q, 4, false),
+      INST(UB, UQ, 4, false),
+      INST(UB, DF, 4, false),
+
+#undef INST
+   };
+
+   if (devinfo.gen < 8)
+      return;
+
+   for (unsigned i = 0; i < sizeof(inst) / sizeof(inst[0]); i++) {
+      if (!devinfo.has_64bit_types && type_sz(inst[i].src_type) == 8)
+         continue;
+
+      brw_MOV(p, retype(g0, inst[i].dst_type), retype(g0, inst[i].src_type));
+      brw_inst_set_dst_hstride(&devinfo, last_inst, inst[i].dst_stride);
+      EXPECT_EQ(inst[i].expected_result, validate(p));
+
+      clear_instructions(p);
+   }
+}
+
 TEST_P(validation_test, half_float_conversion)
 {
    static const struct {