Iago Toral <ito...@igalia.com> writes:
> On Fri, 2019-03-01 at 09:39 +0100, Iago Toral wrote:
>> On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> > 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?
>>
>> We aren't, because the validator is not checking, in general, for
>> accepted type combinations for specific instructions anywhere as far
>> as
>> I can see. If we want to start doing this with HF conversions
>> specifically, I guess it is fine, but in case it is not clear, I
>> think
>> it won't bring any immediate benefits with the
>> VK_KHR_shader_float16_int8 implementation since this series only ever
>> emits conversions involving HF operands via MOV instructions, which
>> is
>> why I thought that validating that no direct MOV conversions from
>> DF/Q
>> types ever happen was useful, since we have code in the driver to
>> handle this scenario.
>>
>> > > > 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
>>
>> I thought these instructions were not valid. I looked again at the
>> BDW
>> PRM and indeed, it lists that HF destination for ADD is only allowed
>> with HF source. The same for all other instructions that can take HF
>> except (S)MOV. However, I have just now realized that the SKL+ PRM is
>> different and it allows ADD to do this, but as far as I can see, ADD
>> is
>> the only exception: you cannot do this with MUL, MAD or MATH for
>> example. I am not sure what to make of this, but I guess that if we
>> want to validate this it doesn't hurt to be safe and assume that we
>> can
>> have more instructions doing implicit conversions.
>>
>> If course, there is the fact that we never emit these kind of
>> conversions anyway, at least not with this series (we don't even use
>> mixed-float mode) but I guess it is fine to add that validation if
>> only
>> to be safe in the future if we ever decide to start using the extra
>> flexibility allowed by the hardware.
>
> I am just now realizing that in Align16 mode, we always expect
> destination horizontal stride to be exactly 1, no matter the type. This
> means that Align16 mixed float instructions with HF destination
> (implicit conversion from F->HF) would also require this (actually the
> docs make this requirement explicit for mixed float instructions
> anyway). This would make it impossible to satisfy the rule for DWord
> alignment on F->HF conversions for implicit conversions in mixed float
> mode instructions. I guess these conversion restrictions are formulated
> specifically for Align1 and should be skipped for Align16, even if that
> is not explicitly stated in the docs. Does that make sense?
>
Yes, that's what I would assume. The hardware spec is seriously
contradictory indeed...
>>
>> > > 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?
>>
>> The restrictions based on operand types that are checked in the
>> validator are specific to Byte or cases where the execution type is
>> larger than the destination stride, for which mixed float has a
>> different set of restrictions.
>>
>> For example, for mixed float we have this rule:
>>
>> "In Align1, destination stride can be smaller than execution type"
>>
>> Which overrides this other from "General restrictions based on
>> operand
>> types":
>>
>> "Destination stride must be equal to the ratio of the sizes of the
>> execution data type to the destination type"
>>
>> So I thought that it would make things easier to keep all
>> restrictions
>> for mixed float separate and make sure that we skipped mixed float
>> instructions in general_restrictions_based_on_operand_types() so we
>> avoid having to add code to skip individual general restrictions that
>> that are overriden for mixed float mode anyway.
>>
>> Anyway, I'll try to rework the patches to do more generic validation
>> of
>> HF conversions like you ask and see what kind of code we end up with.
>>
>> > > > > > > + ((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
