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

Mon Mar 4 23:36:52 UTC 2019

Iago Toral <itoral at igalia.com> writes:

> On Fri, 2019-03-01 at 19:04 -0800, Francisco Jerez wrote:
>> 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()?
>
> I guess this could be arguable, but I was not considering conversion
> MOVs to be mixed-float instructions. There are two reasons for this:
>
> A conversion MOV involving F/HF doesn't seem to be fundamentally
> different from any other conversion instruction involving other types,
> other than the requirement of aligning the destination to a Dword,
> which is not a resriction explictly made for mixed-float mode.
>
> Then, for mixed-float mode, there is this other rule:
>
> "In Align1, destination stride can be smaller than execution type. When
> destination is stride of 1, 16 bit packed data
> is updated on the destination. However, output packed f16 data must be
> oword aligned, no oword crossing in
> packed f16."
>
> Which contradicts the other rule that conversions from F to HF need to
> be DWord aligned on the destination.
>
> So it seems to me that conversion MOVs are not following the same
> principles of mixed-float instructions and we should skip validating
> mixed-float restrictions for them. What do you think?
>

That all seems fairly ambiguous...  And the restriction on DWORD
alignment for conversions includes a mixed-mode ADD instruction among
the examples, so I'm skeptical that MOV could be truly fundamentally
different.

>> > 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/20190304/f5c899e9/attachment.sig>