[v4,35/40] intel/compiler: validate conversions between 64-bit and 8-bit types

Submitted by Iago Toral Quiroga on Feb. 12, 2019, 11:56 a.m.

Details

Message ID 20190212115607.21467-36-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 16 15 14 13 12 11 10 9 8 7 6 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Feb. 12, 2019, 11:56 a.m.
---
 src/intel/compiler/brw_eu_validate.c    | 10 +++++-
 src/intel/compiler/test_eu_validate.cpp | 46 +++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c
index 203641fecb9..b1fdd1ce941 100644
--- a/src/intel/compiler/brw_eu_validate.c
+++ b/src/intel/compiler/brw_eu_validate.c
@@ -533,10 +533,18 @@  general_restrictions_based_on_operand_types(const struct gen_device_info *devinf
 
    /* From the BDW+ PRM:
     *
-    *    "There is no direct conversion from HF to DF or DF to HF.
+    *    "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.
+    *     There is no direct conversion from HF to DF or DF to HF.
     *     There is no direct conversion from HF to Q/UQ or Q/UQ to HF."
     */
    enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
+
+   ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
+            ((dst_type_size == 1 && type_sz(src0_type) == 8) ||
+             (dst_type_size == 8 && type_sz(src0_type) == 1)),
+            "There are no direct conversion between 64-bit types and B/UB");
+
    ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
             ((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) == 8) ||
              (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),
diff --git a/src/intel/compiler/test_eu_validate.cpp b/src/intel/compiler/test_eu_validate.cpp
index 1557b6d2452..06beb53eb5d 100644
--- a/src/intel/compiler/test_eu_validate.cpp
+++ b/src/intel/compiler/test_eu_validate.cpp
@@ -848,6 +848,52 @@  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(B,  Q, 2, false),
+      INST(B, UQ, 2, false),
+      INST(B, DF, 2, false),
+
+      INST(B,  Q, 4, false),
+      INST(B, UQ, 4, false),
+      INST(B, 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 {

Comments

On Tue, Feb 12, 2019 at 5:56 AM Iago Toral Quiroga <itoral@igalia.com>
wrote:

> ---
>  src/intel/compiler/brw_eu_validate.c    | 10 +++++-
>  src/intel/compiler/test_eu_validate.cpp | 46 +++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c
> b/src/intel/compiler/brw_eu_validate.c
> index 203641fecb9..b1fdd1ce941 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -533,10 +533,18 @@ general_restrictions_based_on_operand_types(const
> struct gen_device_info *devinf
>
>     /* From the BDW+ PRM:
>      *
> -    *    "There is no direct conversion from HF to DF or DF to HF.
> +    *    "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.
> +    *     There is no direct conversion from HF to DF or DF to HF.
>      *     There is no direct conversion from HF to Q/UQ or Q/UQ to HF."
>      */
>     enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
> +
> +   ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> +            ((dst_type_size == 1 && type_sz(src0_type) == 8) ||
> +             (dst_type_size == 8 && type_sz(src0_type) == 1)),
> +            "There are no direct conversion between 64-bit types and
> B/UB");
> +
>     ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
>              ((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) ==
> 8) ||
>               (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),
> diff --git a/src/intel/compiler/test_eu_validate.cpp
> b/src/intel/compiler/test_eu_validate.cpp
> index 1557b6d2452..06beb53eb5d 100644
> --- a/src/intel/compiler/test_eu_validate.cpp
> +++ b/src/intel/compiler/test_eu_validate.cpp
> @@ -848,6 +848,52 @@ 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(B,  Q, 2, false),
> +      INST(B, UQ, 2, false),
> +      INST(B, DF, 2, false),
> +
> +      INST(B,  Q, 4, false),
> +      INST(B, UQ, 4, false),
> +      INST(B, DF, 4, false),
>

Probably want some tests with a B or UB source too. :-)  With those added,

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


> +
> +#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 {
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Sat, 2019-02-16 at 09:42 -0600, Jason Ekstrand wrote:
> On Tue, Feb 12, 2019 at 5:56 AM Iago Toral Quiroga <itoral@igalia.com
> > wrote:
> > ---
> > 
> >  src/intel/compiler/brw_eu_validate.c    | 10 +++++-
> > 
> >  src/intel/compiler/test_eu_validate.cpp | 46
> > +++++++++++++++++++++++++
> > 
> >  2 files changed, 55 insertions(+), 1 deletion(-)
> > 
> > 
> > 
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> > 
> > index 203641fecb9..b1fdd1ce941 100644
> > 
> > --- a/src/intel/compiler/brw_eu_validate.c
> > 
> > +++ b/src/intel/compiler/brw_eu_validate.c
> > 
> > @@ -533,10 +533,18 @@
> > general_restrictions_based_on_operand_types(const struct
> > gen_device_info *devinf
> > 
> > 
> > 
> >     /* From the BDW+ PRM:
> > 
> >      *
> > 
> > -    *    "There is no direct conversion from HF to DF or DF to HF.
> > 
> > +    *    "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.
> > 
> > +    *     There is no direct conversion from HF to DF or DF to HF.
> > 
> >      *     There is no direct conversion from HF to Q/UQ or Q/UQ to
> > HF."
> > 
> >      */
> > 
> >     enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
> > inst);
> > 
> > +
> > 
> > +   ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> > 
> > +            ((dst_type_size == 1 && type_sz(src0_type) == 8) ||
> > 
> > +             (dst_type_size == 8 && type_sz(src0_type) == 1)),
> > 
> > +            "There are no direct conversion between 64-bit types
> > and B/UB");
> > 
> > +
> > 
> >     ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> > 
> >              ((dst_type == BRW_REGISTER_TYPE_HF &&
> > type_sz(src0_type) == 8) ||
> > 
> >               (dst_type_size == 8 && src0_type ==
> > BRW_REGISTER_TYPE_HF)),
> > 
> > diff --git a/src/intel/compiler/test_eu_validate.cpp
> > b/src/intel/compiler/test_eu_validate.cpp
> > 
> > index 1557b6d2452..06beb53eb5d 100644
> > 
> > --- a/src/intel/compiler/test_eu_validate.cpp
> > 
> > +++ b/src/intel/compiler/test_eu_validate.cpp
> > 
> > @@ -848,6 +848,52 @@ 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(B,  Q, 2, false),
> > 
> > +      INST(B, UQ, 2, false),
> > 
> > +      INST(B, DF, 2, false),
> > 
> > +
> > 
> > +      INST(B,  Q, 4, false),
> > 
> > +      INST(B, UQ, 4, false),
> > 
> > +      INST(B, DF, 4, false),
> 
> Probably want some tests with a B or UB source too. :-)  With those
> added,

Sure, I added UB variants for all tests locally.
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> 
> > +
> > 
> > +#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 {
> >
Iago Toral Quiroga <itoral@igalia.com> writes:

> ---
>  src/intel/compiler/brw_eu_validate.c    | 10 +++++-
>  src/intel/compiler/test_eu_validate.cpp | 46 +++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_eu_validate.c b/src/intel/compiler/brw_eu_validate.c
> index 203641fecb9..b1fdd1ce941 100644
> --- a/src/intel/compiler/brw_eu_validate.c
> +++ b/src/intel/compiler/brw_eu_validate.c
> @@ -533,10 +533,18 @@ general_restrictions_based_on_operand_types(const struct gen_device_info *devinf
>  
>     /* From the BDW+ PRM:
>      *
> -    *    "There is no direct conversion from HF to DF or DF to HF.
> +    *    "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.
> +    *     There is no direct conversion from HF to DF or DF to HF.
>      *     There is no direct conversion from HF to Q/UQ or Q/UQ to HF."
>      */
>     enum brw_reg_type src0_type = brw_inst_src0_type(devinfo, inst);
> +
> +   ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> +            ((dst_type_size == 1 && type_sz(src0_type) == 8) ||
> +             (dst_type_size == 8 && type_sz(src0_type) == 1)),
> +            "There are no direct conversion between 64-bit types and B/UB");
> +

Same comment here as for the last patch -- Why is this only handling the
MOV instruction?

>     ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
>              ((dst_type == BRW_REGISTER_TYPE_HF && type_sz(src0_type) == 8) ||
>               (dst_type_size == 8 && src0_type == BRW_REGISTER_TYPE_HF)),
> diff --git a/src/intel/compiler/test_eu_validate.cpp b/src/intel/compiler/test_eu_validate.cpp
> index 1557b6d2452..06beb53eb5d 100644
> --- a/src/intel/compiler/test_eu_validate.cpp
> +++ b/src/intel/compiler/test_eu_validate.cpp
> @@ -848,6 +848,52 @@ 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(B,  Q, 2, false),
> +      INST(B, UQ, 2, false),
> +      INST(B, DF, 2, false),
> +
> +      INST(B,  Q, 4, false),
> +      INST(B, UQ, 4, false),
> +      INST(B, 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 {
> -- 
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, 2019-02-26 at 15:50 -0800, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral@igalia.com> writes:
> 
> > ---
> >  src/intel/compiler/brw_eu_validate.c    | 10 +++++-
> >  src/intel/compiler/test_eu_validate.cpp | 46
> > +++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> > index 203641fecb9..b1fdd1ce941 100644
> > --- a/src/intel/compiler/brw_eu_validate.c
> > +++ b/src/intel/compiler/brw_eu_validate.c
> > @@ -533,10 +533,18 @@
> > general_restrictions_based_on_operand_types(const struct
> > gen_device_info *devinf
> >  
> >     /* From the BDW+ PRM:
> >      *
> > -    *    "There is no direct conversion from HF to DF or DF to HF.
> > +    *    "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.
> > +    *     There is no direct conversion from HF to DF or DF to HF.
> >      *     There is no direct conversion from HF to Q/UQ or Q/UQ to
> > HF."
> >      */
> >     enum brw_reg_type src0_type = brw_inst_src0_type(devinfo,
> > inst);
> > +
> > +   ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> > +            ((dst_type_size == 1 && type_sz(src0_type) == 8) ||
> > +             (dst_type_size == 8 && type_sz(src0_type) == 1)),
> > +            "There are no direct conversion between 64-bit types
> > and B/UB");
> > +
> 
> Same comment here as for the last patch -- Why is this only handling
> the
> MOV instruction?

As in the last patch, these restrictions are included in the
programming notes for the MOV instructions, so they are specific to it.

> 
> >     ERROR_IF(brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV &&
> >              ((dst_type == BRW_REGISTER_TYPE_HF &&
> > type_sz(src0_type) == 8) ||
> >               (dst_type_size == 8 && src0_type ==
> > BRW_REGISTER_TYPE_HF)),
> > diff --git a/src/intel/compiler/test_eu_validate.cpp
> > b/src/intel/compiler/test_eu_validate.cpp
> > index 1557b6d2452..06beb53eb5d 100644
> > --- a/src/intel/compiler/test_eu_validate.cpp
> > +++ b/src/intel/compiler/test_eu_validate.cpp
> > @@ -848,6 +848,52 @@ 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(B,  Q, 2, false),
> > +      INST(B, UQ, 2, false),
> > +      INST(B, DF, 2, false),
> > +
> > +      INST(B,  Q, 4, false),
> > +      INST(B, UQ, 4, false),
> > +      INST(B, 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 {
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev