<html dir="ltr"><head></head><body style="text-align:left; direction:ltr;"><div>On Sat, 2019-02-16 at 09:40 -0600, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 12, 2019 at 11:53 AM Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">---<br>
 src/intel/compiler/brw_eu_validate.c    |  64 ++++++++++++-<br>
 src/intel/compiler/test_eu_validate.cpp | 122 ++++++++++++++++++++++++<br>
 2 files changed, 185 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c<br>
index 000a05cb6ac..203641fecb9 100644<br>
--- a/src/intel/compiler/brw_eu_validate.c<br>
+++ b/src/intel/compiler/brw_eu_validate.c<br>
@@ -531,7 +531,69 @@ general_restrictions_based_on_operand_types(const struct gen_device_info *devinf<br>
        exec_type_size == 8 && dst_type_size == 4)<br>
       dst_type_size = 8;<br>
<br>
-   if (exec_type_size > dst_type_size) {<br>
+   /* From the BDW+ PRM:<br>
+    *<br>
+    *    "There is no direct conversion from HF to DF or DF to HF.<br>
+    *     There is no direct conversion from HF to Q/UQ or Q/UQ to HF."<br>
+    */<br>
+   enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);<br>
+   ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&<br>
+            ((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) == 8) ||<br>
+             (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),<br>
+            "There are no direct conversion between 64-bit types and HF");<br>
+<br>
+   /* From the BDW+ PRM:<br>
+    *<br>
+    *   "Conversion between Integer and HF (Half Float) must be<br>
+    *    DWord-aligned and strided by a DWord on the destination."<br>
+    *<br>
+    * But this seems to be expanded on CHV and SKL+ by:<br>
+    *<br>
+    *   "There is a relaxed alignment rule for word destinations. When<br>
+    *    the destination type is word (UW, W, HF), destination data types<br>
+    *    can be aligned to either the lowest word or the second lowest<br>
+    *    word of the execution channel. This means the destination data<br>
+    *    words can be either all in the even word locations or all in the<br>
+    *    odd word locations."<br>
+    *<br>
+    * We do not implement the second rule as is though, since empirical testing<br>
+    * shows inconsistencies:<br>
+    *   - It suggests that packed 16-bit is not allowed, which is not true.<br>
+    *   - It suggests that conversions from Q/DF to W (which need to be 64-bit<br>
+    *     aligned on the destination) are not possible, which is not true.<br>
+    *   - It suggests that conversions from 16-bit executions types to W need<br>
+    *     to be 32-bit aligned, which doesn't seem to be necessary.<br>
+    *<br>
+    * So from this rule we only validate the implication that conversion from<br>
+    * F to HF needs to be DWord aligned too (in BDW this is limited to<br>
+    * conversions from integer types).<br>
+    */<br>
+   bool is_half_float_conversion =<br>
+       brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&<br>
+       dst_type != src0_type &&<br>
+       (dst_type == BRW_REGISTER_TYPE_HF || src0_type == BRW_REGISTER_TYPE_HF);<br>
+<br>
+   if (is_half_float_conversion) {<br>
+      assert(devinfo->gen >= 8);<br>
+<br>
+      if ((dst_type == BRW_REGISTER_TYPE_HF && brw_reg_type_is_integer(src0_type)) ||<br>
+          (brw_reg_type_is_integer(dst_type) && src0_type == BRW_REGISTER_TYPE_HF)) {<br>
+         ERROR_IF(dst_stride * dst_type_size != 4,<br>
+                  "Conversions between integer and half-float must be strided "<br>
+                  "by a DWord on the destination");<br></blockquote><div><br></div><div>Does this mean stride must be 4B or does it mean a multiple of 4B?<br></div><div> </div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+<br>
+         unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo, inst);<br>
+         ERROR_IF(subreg % 4 != 0,<br>
+                  "Conversions between integer and half-float must be aligned "<br>
+                  "to a DWord on the destination");<br>
+      } else if ((devinfo->is_cherryview || devinfo->gen >= 9) &&<br>
+                 dst_type == BRW_REGISTER_TYPE_HF) {<br>
+         ERROR_IF(dst_stride != 2,<br></blockquote><div><br></div><div>Should this be dst_stride != 2 or dst_stride == 1?  If dst_stride were, say 4, that would place them all in even or all in odd locations.  It's only if dst_stride == 1 that you end up with both even and odd.<br></div></div></div></blockquote><div><br></div><div>I think this needs to be exactly a DWord for both the stride and the alignment. When Curro explained this he made the case that what is probably happening under the hood is that there is a promotion of the exec type to 32-bit, and then the following general restriction applies:</div><div><br></div><div>"When the Execution Data Type is wider than the destination data type, the destination must</div><div>be aligned as required by the wider execution data type and specify a HorzStride equal to</div><div>the ratio in sizes of the two data types. For example, a mov with a D source and B destination</div><div>must use a 4-byte aligned destination and a Dst.HorzStride of 4"</div><div><br></div><div>Which is described in the same terms (requiring exact stride and alignment to match the execution type).</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
+                  "Conversions to HF must have either all words in even word "<br>
+                  "locations or all words in odd word locations");<br>
+      }<br>
+<br>
+   } else if (exec_type_size > dst_type_size) {<br>
       if (!(dst_type_is_byte && inst_is_raw_move(devinfo, inst))) {<br>
          ERROR_IF(dst_stride * dst_type_size != exec_type_size,<br>
                   "Destination stride must be equal to the ratio of the sizes "<br>
diff --git a/src/intel/compiler/test_eu_validate.cpp b/src/intel/compiler/test_eu_validate.cpp<br>
index 73300b23122..1557b6d2452 100644<br>
--- a/src/intel/compiler/test_eu_validate.cpp<br>
+++ b/src/intel/compiler/test_eu_validate.cpp<br>
@@ -848,6 +848,128 @@ TEST_P(validation_test, byte_destination_relaxed_alignment)<br>
    }<br>
 }<br>
<br>
+TEST_P(validation_test, half_float_conversion)<br>
+{<br>
+   static const struct {<br>
+      enum brw_reg_type dst_type;<br>
+      enum brw_reg_type src_type;<br>
+      unsigned dst_stride;<br>
+      unsigned dst_subnr;<br>
+      bool expected_result;<br>
+   } inst[] = {<br>
+#define INST(dst_type, src_type, dst_stride, dst_subnr, expected_result)  \<br>
+      {                                                                   \<br>
+         BRW_REGISTER_TYPE_##dst_type,                                    \<br>
+         BRW_REGISTER_TYPE_##src_type,                                    \<br>
+         BRW_HORIZONTAL_STRIDE_##dst_stride,                              \<br>
+         dst_subnr,                                                       \<br>
+         expected_result,                                                 \<br>
+      }<br>
+<br>
+      /* MOV to half-float destination (F source handled later) */<br>
+      INST(HF,  B, 1, 0, false),<br>
+      INST(HF,  W, 1, 0, false),<br>
+      INST(HF, HF, 1, 0, true),<br>
+      INST(HF, HF, 1, 2, true),<br>
+      INST(HF,  D, 1, 0, false),<br>
+      INST(HF,  Q, 1, 0, false),<br>
+      INST(HF, DF, 1, 0, false),<br>
+      INST(HF,  B, 2, 0, true),<br>
+      INST(HF,  B, 2, 2, false),<br>
+      INST(HF,  W, 2, 0, true),<br>
+      INST(HF,  W, 2, 2, false),<br>
+      INST(HF, HF, 2, 0, true),<br>
+      INST(HF, HF, 2, 2, true),<br>
+      INST(HF,  D, 2, 0, true),<br>
+      INST(HF,  D, 2, 2, false),<br>
+      INST(HF,  Q, 2, 0, false),<br>
+      INST(HF, DF, 2, 0, false),<br>
+      INST(HF,  B, 4, 0, false),<br>
+      INST(HF,  W, 4, 0, false),<br>
+      INST(HF, HF, 4, 0, true),<br>
+      INST(HF, HF, 4, 2, true),<br>
+      INST(HF,  D, 4, 0, false),<br>
+      INST(HF,  Q, 4, 0, false),<br>
+      INST(HF, DF, 4, 0, false),<br>
+<br>
+      /* MOV from half-float source */<br>
+      INST( B, HF, 1, 0, false),<br>
+      INST( W, HF, 1, 0, false),<br>
+      INST( D, HF, 1, 0, true),<br>
+      INST( D, HF, 1, 4, true),<br>
+      INST( F, HF, 1, 0, true),<br>
+      INST( F, HF, 1, 4, true),<br>
+      INST( Q, HF, 1, 0, false),<br>
+      INST(DF, HF, 1, 0, false),<br>
+      INST( B, HF, 2, 0, false),<br>
+      INST( W, HF, 2, 0, true),<br>
+      INST( W, HF, 2, 2, false),<br>
+      INST( D, HF, 2, 0, false),<br>
+      INST( F, HF, 2, 0, true),<br>
+      INST( F, HF, 2, 2, true),<br>
+      INST( B, HF, 4, 0, true),<br>
+      INST( B, HF, 4, 1, false),<br>
+      INST( W, HF, 4, 0, false),<br>
+<br>
+#undef INST<br>
+   };<br>
+<br>
+   if (devinfo.gen < 8)<br>
+      return;<br>
+<br>
+   for (unsigned i = 0; i < sizeof(inst) / sizeof(inst[0]); i++) {<br>
+      if (!devinfo.has_64bit_types &&<br>
+          (type_sz(inst[i].src_type) == 8 || type_sz(inst[i].dst_type) == 8)) {<br>
+         continue;<br>
+      }<br>
+<br>
+      brw_MOV(p, retype(g0, inst[i].dst_type), retype(g0, inst[i].src_type));<br>
+<br>
+      brw_inst_set_exec_size(&devinfo, last_inst, BRW_EXECUTE_4);<br>
+<br>
+      brw_inst_set_dst_hstride(&devinfo, last_inst, inst[i].dst_stride);<br>
+      brw_inst_set_dst_da1_subreg_nr(&devinfo, last_inst, inst[i].dst_subnr);<br>
+<br>
+      if (inst[i].src_type == BRW_REGISTER_TYPE_B) {<br>
+         brw_inst_set_src0_vstride(&devinfo, last_inst, BRW_VERTICAL_STRIDE_4);<br>
+         brw_inst_set_src0_width(&devinfo, last_inst, BRW_WIDTH_2);<br>
+         brw_inst_set_src0_hstride(&devinfo, last_inst, BRW_HORIZONTAL_STRIDE_2);<br>
+      } else {<br>
+         brw_inst_set_src0_vstride(&devinfo, last_inst, BRW_VERTICAL_STRIDE_4);<br>
+         brw_inst_set_src0_width(&devinfo, last_inst, BRW_WIDTH_4);<br>
+         brw_inst_set_src0_hstride(&devinfo, last_inst, BRW_HORIZONTAL_STRIDE_1);<br>
+      }<br>
+<br>
+      EXPECT_EQ(inst[i].expected_result, validate(p));<br>
+<br>
+      clear_instructions(p);<br>
+   }<br>
+<br>
+   /* Conversion from F to HF has Dword destination alignment restriction<br>
+    * on CHV and SKL+ only<br>
+    */<br>
+   brw_MOV(p, retype(g0, BRW_REGISTER_TYPE_HF),<br>
+              retype(g0, BRW_REGISTER_TYPE_F));<br>
+<br>
+   brw_inst_set_dst_hstride(&devinfo, last_inst, BRW_HORIZONTAL_STRIDE_1);<br>
+<br>
+   if (devinfo.gen >= 9 || devinfo.is_cherryview) {<br>
+      EXPECT_FALSE(validate(p));<br>
+   } else {<br>
+      assert(devinfo.gen == 8);<br>
+      EXPECT_TRUE(validate(p));<br>
+   }<br>
+   clear_instructions(p);<br>
+<br>
+   brw_MOV(p, retype(g0, BRW_REGISTER_TYPE_HF),<br>
+              retype(g0, BRW_REGISTER_TYPE_F));<br>
+<br>
+   brw_inst_set_dst_hstride(&devinfo, last_inst, BRW_HORIZONTAL_STRIDE_2);<br>
+<br>
+   EXPECT_TRUE(validate(p));<br>
+   clear_instructions(p);<br>
+}<br>
+<br>
 TEST_P(validation_test, vector_immediate_destination_alignment)<br>
 {<br>
    static const struct {<br>
</blockquote></div></div></blockquote></body></html>