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

[Francisco Jerez]

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.

> 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.  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.  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, if anything because currently
it will reject working code that uses HF types.

> ---
>  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
-------------- 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/20190123/22bc4132/attachment.sig>