[Mesa-dev] [PATCH] intel/compiler: update validator to account for half-float exec type promotion

[Iago Toral]

On Wed, 2019-01-23 at 06:03 -0800, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > Commit c84ec70b3a72 implemented execution type promotion to 32-bit
> > for
> > conversions involving half-float registers, which empirical testing
> > suggested
> > was required, but it did not incorporate this change into the
> > assembly validator
> > logic. This commits adds that, preventing validation errors like
> > this:
> > 
> 
> I don't think we should be validating empirical assumptions in the EU
> validator.

I am not sure I get your point, isn't c84ec70b3a72 also based on
empirical testing after all?


> > mov(16)  g9<4>B   g3<16,8,2>HF     { align1 1H };
> > ERROR: Destination stride must be equal to the ratio of the sizes
> > of the
> >        execution data type to the destination type
> > 
> > Fixes: c84ec70b3a72 "intel/fs: Promote execution type to 32-bit
> > when any half-float conversion is needed."
> 
> I don't think this "fixes" anything that ever worked.

It is true that the code in that trace above is not something we can
produce right now, because it is a conversion from HF to B and that
should only happen within the context of VK_KHR_shader_float16_int8,
however, this is a consequence of the fact that since c84ec70b3a72
there is an inconsistency between what we do at the IR level regarding
execution size of HF conversions and what the EU validator is doing,
and from that perspective this is really fixing an inconsistency that
didn't exist before, and I thought we would want to address that sooner
rather than later and track it down to the original change that
introduced that inconsistency so we know where this is coming from.

Anyway, that was my rationale for the Fixes tag, but if you think this
is not useful I am happy to drop this patch and just include it as part
of my series without the tag.

>   The validator is
> still missing an implementation of the quirky HF restrictions, and it
> wasn't the purpose of c84ec70b3a72 to do such a thing.

While this is true in general, the EU validator does consider the
execution type of the instruction to validate general rules such as the
one I mentioned in the commit message in this patch. And that part of
the validator is inconsistent with c84ec70b3a72. In fact, the EU
validator is accounting for execution size promotion of HF instructions
to 32-bit in SKL+ and CHV only, for conversions from HF->F and mixed
float mode instructions... which is part of what c84ec70b3a72 addresses
at the IR level, which it actually does for all hardware platforms and
in more cases.

>   You *should*
> definitely implement those restrictions (as they're stated in the
> hardware spec, without empirical assumptions) in the validator as
> part
> of your VK_KHR_shader_float16_int8 series,

Again, I am not sure what you mean by "without empirical assumptions".
According to the commit message in c84ec70b3a72 "the docs are fairly
imcomplete and inconsistent" and you explained here that you had to do
some experimentation of your own using the simulator (where you found
its results to also be inconsistent with the hardware docs) to try and
guess what is happening. That's why I was using the term "empirical"
here, since the hardware docs alone aren't clear or correct enough to
understand what is really happening, when and in what platforms.

Anyway, if you don't like the term "empirical" to refer to all this,
that's fine by me, but what I need to know is if we agree that we need
to implement the same type promotion rules in the validator that we are
implementing in the IR, indepedently of whether refer to them as "based
on empirical testing" or not.

>  if anything because currently
> it will reject working code that uses HF types.

Just for the sake of clarity, since that sentence above could be
interpreted as if all HF code would be rejected: we have been using HF
types since we landed VK_KHR_16bit_storage, which includes conversions
between HF and F and the EU validator is not complaining about any of
that. It is currently a problem only with conversions to smaller types
(so only conversions to Byte) because that's where we check for that
restriction on the stride, which is based on the execution type of the
instruction.

> 
> > ---
> >  src/intel/compiler/brw_eu_validate.c | 27 ++++++++++++++--------
> > -----
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/intel/compiler/brw_eu_validate.c
> > b/src/intel/compiler/brw_eu_validate.c
> > index a25010b225c..3bb37677672 100644
> > --- a/src/intel/compiler/brw_eu_validate.c
> > +++ b/src/intel/compiler/brw_eu_validate.c
> > @@ -325,17 +325,20 @@ execution_type(const struct gen_device_info
> > *devinfo, const brw_inst *inst)
> >     unsigned num_sources = num_sources_from_inst(devinfo, inst);
> >     enum brw_reg_type src0_exec_type, src1_exec_type;
> >  
> > -   /* Execution data type is independent of destination data type,
> > except in
> > -    * mixed F/HF instructions on CHV and SKL+.
> > +   /* Empirical testing suggests that type conversions involving
> > half-float
> > +    * promote execution type to 32-bit. See get_exec_type() in
> > brw_ir_fs.h.
> >      */
> >     enum brw_reg_type dst_exec_type = brw_inst_dst_type(devinfo,
> > inst);
> >  
> >     src0_exec_type =
> > execution_type_for_type(brw_inst_src0_type(devinfo, inst));
> >     if (num_sources == 1) {
> > -      if ((devinfo->gen >= 9 || devinfo->is_cherryview) &&
> > -          src0_exec_type == BRW_REGISTER_TYPE_HF) {
> > -         return dst_exec_type;
> > +      if (type_sz(src0_exec_type) == 2 && dst_exec_type !=
> > src0_exec_type) {
> > +         if (src0_exec_type == BRW_REGISTER_TYPE_HF)
> > +            return BRW_REGISTER_TYPE_F;
> > +         else if (dst_exec_type == BRW_REGISTER_TYPE_HF)
> > +            return BRW_REGISTER_TYPE_D;
> >        }
> > +
> >        return src0_exec_type;
> >     }
> >  
> > @@ -367,14 +370,12 @@ execution_type(const struct gen_device_info
> > *devinfo, const brw_inst *inst)
> >         src1_exec_type == BRW_REGISTER_TYPE_DF)
> >        return BRW_REGISTER_TYPE_DF;
> >  
> > -   if (devinfo->gen >= 9 || devinfo->is_cherryview) {
> > -      if (dst_exec_type == BRW_REGISTER_TYPE_F ||
> > -          src0_exec_type == BRW_REGISTER_TYPE_F ||
> > -          src1_exec_type == BRW_REGISTER_TYPE_F) {
> > -         return BRW_REGISTER_TYPE_F;
> > -      } else {
> > -         return BRW_REGISTER_TYPE_HF;
> > -      }
> > +   if (dst_exec_type == BRW_REGISTER_TYPE_F ||
> > +       src0_exec_type == BRW_REGISTER_TYPE_F ||
> > +       src1_exec_type == BRW_REGISTER_TYPE_F) {
> > +      return BRW_REGISTER_TYPE_F;
> > +   } else {
> > +      return BRW_REGISTER_TYPE_HF;
> >     }
> >  
> >     assert(src0_exec_type == BRW_REGISTER_TYPE_F);
> > -- 
> > 2.17.1