On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote: > Iago Toral Quiroga <ito...@igalia.com> writes: > > > --- > > src/intel/compiler/brw_eu_validate.c | 64 ++++++++++++- > > src/intel/compiler/test_eu_validate.cpp | 122 > > ++++++++++++++++++++++++ > > 2 files changed, 185 insertions(+), 1 deletion(-) > > > > diff --git a/src/intel/compiler/brw_eu_validate.c > > b/src/intel/compiler/brw_eu_validate.c > > index 000a05cb6ac..203641fecb9 100644 > > --- a/src/intel/compiler/brw_eu_validate.c > > +++ b/src/intel/compiler/brw_eu_validate.c > > @@ -531,7 +531,69 @@ > > 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 (exec_type_size > dst_type_size) { > > + /* From the BDW+ PRM: > > + * > > + * "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 && > > Why is only the MOV instruction handled here and below? Aren't other > instructions able to do implicit conversions? Probably means you > need > to deal with two sources rather than one.
This comes from the programming notes of the MOV instruction (Volume 2a, Command Reference - Instructions - MOV), so it is described specifically for the MOV instruction. I should probably have made this clear in the comment. > > + ((dst_type == BRW_REGISTER_TYPE_HF && > > type_sz(src0_type) == 8) || > > + (dst_type_size == 8 && src0_type == > > BRW_REGISTER_TYPE_HF)), > > + "There are no direct conversion between 64-bit types > > and HF"); > > + > > + /* From the BDW+ PRM: > > + * > > + * "Conversion between Integer and HF (Half Float) must be > > + * DWord-aligned and strided by a DWord on the destination." > > + * > > + * But this seems to be expanded on CHV and SKL+ by: > > + * > > + * "There is a relaxed alignment rule for word destinations. > > When > > + * the destination type is word (UW, W, HF), destination > > data types > > + * can be aligned to either the lowest word or the second > > lowest > > + * word of the execution channel. This means the destination > > data > > + * words can be either all in the even word locations or all > > in the > > + * odd word locations." > > + * > > + * We do not implement the second rule as is though, since > > empirical testing > > + * shows inconsistencies: > > + * - It suggests that packed 16-bit is not allowed, which is > > not true. > > + * - It suggests that conversions from Q/DF to W (which need > > to be 64-bit > > + * aligned on the destination) are not possible, which is > > not true. > > + * - It suggests that conversions from 16-bit executions > > types to W need > > + * to be 32-bit aligned, which doesn't seem to be > > necessary. > > + * > > + * So from this rule we only validate the implication that > > conversion from > > + * F to HF needs to be DWord aligned too (in BDW this is > > limited to > > + * conversions from integer types). > > + */ > > + bool is_half_float_conversion = > > + brw_inst_opcode(devinfo, inst) == BRW_OPCODE_MOV && > > + dst_type != src0_type && > > + (dst_type == BRW_REGISTER_TYPE_HF || src0_type == > > BRW_REGISTER_TYPE_HF); > > + > > + if (is_half_float_conversion) { > > + assert(devinfo->gen >= 8); > > + > > + if ((dst_type == BRW_REGISTER_TYPE_HF && > > brw_reg_type_is_integer(src0_type)) || > > + (brw_reg_type_is_integer(dst_type) && src0_type == > > BRW_REGISTER_TYPE_HF)) { > > + ERROR_IF(dst_stride * dst_type_size != 4, > > + "Conversions between integer and half-float must > > be strided " > > + "by a DWord on the destination"); > > + > > + unsigned subreg = brw_inst_dst_da1_subreg_nr(devinfo, > > inst); > > + ERROR_IF(subreg % 4 != 0, > > + "Conversions between integer and half-float must > > be aligned " > > + "to a DWord on the destination"); > > + } else if ((devinfo->is_cherryview || devinfo->gen >= 9) && > > + dst_type == BRW_REGISTER_TYPE_HF) { > > + ERROR_IF(dst_stride != 2, > > + "Conversions to HF must have either all words in > > even word " > > + "locations or all words in odd word locations"); > > + } > > + > > + } else if (exec_type_size > dst_type_size) { > > if (!(dst_type_is_byte && inst_is_raw_move(devinfo, inst))) > > { > > ERROR_IF(dst_stride * dst_type_size != exec_type_size, > > "Destination stride must be equal to the ratio > > of the sizes " > > diff --git a/src/intel/compiler/test_eu_validate.cpp > > b/src/intel/compiler/test_eu_validate.cpp > > index 73300b23122..1557b6d2452 100644 > > --- a/src/intel/compiler/test_eu_validate.cpp > > +++ b/src/intel/compiler/test_eu_validate.cpp > > @@ -848,6 +848,128 @@ TEST_P(validation_test, > > byte_destination_relaxed_alignment) > > } > > } > > > > +TEST_P(validation_test, half_float_conversion) > > +{ > > + static const struct { > > + enum brw_reg_type dst_type; > > + enum brw_reg_type src_type; > > + unsigned dst_stride; > > + unsigned dst_subnr; > > + bool expected_result; > > + } inst[] = { > > +#define INST(dst_type, src_type, dst_stride, dst_subnr, > > expected_result) \ > > + { > > \ > > + BRW_REGISTER_TYPE_##dst_type, > > \ > > + BRW_REGISTER_TYPE_##src_type, > > \ > > + BRW_HORIZONTAL_STRIDE_##dst_stride, > > \ > > + dst_subnr, > > \ > > + expected_result, > > \ > > + } > > + > > + /* MOV to half-float destination (F source handled later) */ > > + INST(HF, B, 1, 0, false), > > + INST(HF, W, 1, 0, false), > > + INST(HF, HF, 1, 0, true), > > + INST(HF, HF, 1, 2, true), > > + INST(HF, D, 1, 0, false), > > + INST(HF, Q, 1, 0, false), > > + INST(HF, DF, 1, 0, false), > > + INST(HF, B, 2, 0, true), > > + INST(HF, B, 2, 2, false), > > + INST(HF, W, 2, 0, true), > > + INST(HF, W, 2, 2, false), > > + INST(HF, HF, 2, 0, true), > > + INST(HF, HF, 2, 2, true), > > + INST(HF, D, 2, 0, true), > > + INST(HF, D, 2, 2, false), > > + INST(HF, Q, 2, 0, false), > > + INST(HF, DF, 2, 0, false), > > + INST(HF, B, 4, 0, false), > > + INST(HF, W, 4, 0, false), > > + INST(HF, HF, 4, 0, true), > > + INST(HF, HF, 4, 2, true), > > + INST(HF, D, 4, 0, false), > > + INST(HF, Q, 4, 0, false), > > + INST(HF, DF, 4, 0, false), > > + > > + /* MOV from half-float source */ > > + INST( B, HF, 1, 0, false), > > + INST( W, HF, 1, 0, false), > > + INST( D, HF, 1, 0, true), > > + INST( D, HF, 1, 4, true), > > + INST( F, HF, 1, 0, true), > > + INST( F, HF, 1, 4, true), > > + INST( Q, HF, 1, 0, false), > > + INST(DF, HF, 1, 0, false), > > + INST( B, HF, 2, 0, false), > > + INST( W, HF, 2, 0, true), > > + INST( W, HF, 2, 2, false), > > + INST( D, HF, 2, 0, false), > > + INST( F, HF, 2, 0, true), > > + INST( F, HF, 2, 2, true), > > + INST( B, HF, 4, 0, true), > > + INST( B, HF, 4, 1, false), > > + INST( W, HF, 4, 0, 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 || > > type_sz(inst[i].dst_type) == 8)) { > > + continue; > > + } > > + > > + brw_MOV(p, retype(g0, inst[i].dst_type), retype(g0, > > inst[i].src_type)); > > + > > + brw_inst_set_exec_size(&devinfo, last_inst, BRW_EXECUTE_4); > > + > > + brw_inst_set_dst_hstride(&devinfo, last_inst, > > inst[i].dst_stride); > > + brw_inst_set_dst_da1_subreg_nr(&devinfo, last_inst, > > inst[i].dst_subnr); > > + > > + if (inst[i].src_type == BRW_REGISTER_TYPE_B) { > > + brw_inst_set_src0_vstride(&devinfo, last_inst, > > BRW_VERTICAL_STRIDE_4); > > + brw_inst_set_src0_width(&devinfo, last_inst, > > BRW_WIDTH_2); > > + brw_inst_set_src0_hstride(&devinfo, last_inst, > > BRW_HORIZONTAL_STRIDE_2); > > + } else { > > + brw_inst_set_src0_vstride(&devinfo, last_inst, > > BRW_VERTICAL_STRIDE_4); > > + brw_inst_set_src0_width(&devinfo, last_inst, > > BRW_WIDTH_4); > > + brw_inst_set_src0_hstride(&devinfo, last_inst, > > BRW_HORIZONTAL_STRIDE_1); > > + } > > + > > + EXPECT_EQ(inst[i].expected_result, validate(p)); > > + > > + clear_instructions(p); > > + } > > + > > + /* Conversion from F to HF has Dword destination alignment > > restriction > > + * on CHV and SKL+ only > > + */ > > + brw_MOV(p, retype(g0, BRW_REGISTER_TYPE_HF), > > + retype(g0, BRW_REGISTER_TYPE_F)); > > + > > + brw_inst_set_dst_hstride(&devinfo, last_inst, > > BRW_HORIZONTAL_STRIDE_1); > > + > > + if (devinfo.gen >= 9 || devinfo.is_cherryview) { > > + EXPECT_FALSE(validate(p)); > > + } else { > > + assert(devinfo.gen == 8); > > + EXPECT_TRUE(validate(p)); > > + } > > + clear_instructions(p); > > + > > + brw_MOV(p, retype(g0, BRW_REGISTER_TYPE_HF), > > + retype(g0, BRW_REGISTER_TYPE_F)); > > + > > + brw_inst_set_dst_hstride(&devinfo, last_inst, > > BRW_HORIZONTAL_STRIDE_2); > > + > > + EXPECT_TRUE(validate(p)); > > + clear_instructions(p); > > +} > > + > > TEST_P(validation_test, vector_immediate_destination_alignment) > > { > > static const struct { > > -- > > 2.17.1 > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev