Iago Toral Quiroga <[email protected]> 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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
