[Mesa-dev] [PATCH v4 34/40] intel/compiler: validate region restrictions for half-float conversions

Sat Mar 2 03:04:07 UTC 2019

Iago Toral <itoral at igalia.com> writes:

> On Thu, 2019-02-28 at 09:54 -0800, Francisco Jerez wrote:
>> Iago Toral <itoral at igalia.com> writes:
>> 
>> > On Wed, 2019-02-27 at 13:47 -0800, Francisco Jerez wrote:
>> > > Iago Toral <itoral at igalia.com> writes:
>> > > 
>> > > > On Tue, 2019-02-26 at 14:54 -0800, Francisco Jerez wrote:
>> > > > > Iago Toral Quiroga <itoral at 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.

Luckily these type conversion restrictions aren't really specific to any
instruction AFAICT, even though they only seem to be mentioned
explicitly for the MOV instruction...

> 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,

Yes, I can see that's the intention of this series, but how can you make
sure that nothing in the optimizer is breaking your assumption if you
don't add some validator code to verify the claim of your last
paragraph?

> 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.
>
>[...]
>> 
>> 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.
>

I'm fine with that, but it doesn't seem like what this patch is doing...
Isn't it adding code to validate mixed-mode float MOV instructions in
general_restrictions_based_on_operand_types()?

> 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.
>

Thanks.

>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190301/d109775e/attachment.sig>