Iago Toral <ito...@igalia.com> writes: > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote: >> Iago Toral <ito...@igalia.com> writes: >> >> > 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. >> > >> >> Maybe the one above is specified in the MOV page only, probably due >> to >> an oversight (If these restrictions were really specific to the MOV >> instruction, what would prevent you from implementing such >> conversions >> through a different instruction? E.g. "ADD dst:df, src:hf, 0" which >> would be substantially more efficient than what you're doing in PATCH >> 02) > > Instructions that take HF can only be strictly HF or mix F and HF > (mixed mode float), with MOV being the only exception. That means that > any instruction like the one you use above are invalid. Maybe we should > validate explicitly that instructions that use HF are strictly HF or > mixed-float mode only? >
So you're acknowledging that the conversions checked above are illegal whether the instruction is a MOV or something else. Where are we preventing instructions other than MOV with such conversions from being accepted by the validator? >> but I see other restriction checks in this patch which are >> certainly specified in the generic regioning restrictions page and >> you're limiting to the MOV instruction... > > There are two rules below: > > 1. The one about conversions between integer and half-float. Again, > these can only happen through MOV for the same reasons, so I think this > one should be fine. > Why do you think that can only happen through a MOV instruction? The hardware spec lists the following as a valid example in the register region restrictions page: | add (8) r10.0<2>:hf r11.0<8;8,1>:w r12.0<8;8,1>:w > 2. The one about word destinations (of which we are only really > implementing conversions from F->HF). Here the rule is more generic and > I agree that expanding this to include any other mixed float mode > instruction would make sense. However, validation for mixed float mode > has its own set rules, some of which are incompatible with the general > region restrictions being validated here, so I think it is inconvenient > to try and do that validation here (see patch 36 and then patch 37). > What I would propose, if you agree, is that we only implement this for > MOV here, and then for mixed float mode instructions, we implement the > more generic version of this check (that would then go in patch 37). > How does that sound? > I still don't understand why you want to implement the same restriction twice, once for MOV and once for all other mixed-mode instructions. How is that more convenient? >> > > > + ((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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev