[Mesa-dev] [PATCH 09/25] i965: Reorder brw_reg_type enum values

[Scott D Phillips]

Matt Turner <mattst88 at gmail.com> writes:
> These vaguely corresponded to the hardware encodings, but that is purely
> historical at this point. Reorder them so we stop making things "almost
> work" when mixing enums.
>
> The ordering has been closen so that no enum value is the same as a
> compatible hardware encoding.
> ---
>  src/intel/compiler/brw_eu.c      |  1 -
>  src/intel/compiler/brw_eu_emit.c |  6 ------
>  src/intel/compiler/brw_fs.cpp    |  1 +
>  src/intel/compiler/brw_reg.h     | 32 ++++++++++++++------------------
>  src/intel/compiler/brw_vec4.cpp  |  3 ++-
>  5 files changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu.c b/src/intel/compiler/brw_eu.c
> index 0ef52e219c..700a1badd4 100644
> --- a/src/intel/compiler/brw_eu.c
> +++ b/src/intel/compiler/brw_eu.c
> @@ -62,7 +62,6 @@ brw_reg_type_letters(unsigned type)
>        [BRW_REGISTER_TYPE_UQ] = "UQ",
>        [BRW_REGISTER_TYPE_Q]  = "Q",
>     };
> -   assert(type <= BRW_REGISTER_TYPE_Q);
>     return names[type];
>  }
>  
> diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
> index 6673e0741a..b59fc33a54 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -112,7 +112,6 @@ brw_reg_type_to_hw_type(const struct gen_device_info *devinfo,
>        };
>        assert(type < ARRAY_SIZE(imm_hw_types));
>        assert(imm_hw_types[type] != -1);
> -      assert(devinfo->gen >= 8 || type < BRW_REGISTER_TYPE_DF);
>        return imm_hw_types[type];
>     } else {
>        /* Non-immediate registers */
> @@ -134,8 +133,6 @@ brw_reg_type_to_hw_type(const struct gen_device_info *devinfo,
>        };
>        assert(type < ARRAY_SIZE(hw_types));
>        assert(hw_types[type] != -1);
> -      assert(devinfo->gen >= 7 || type < BRW_REGISTER_TYPE_DF);
> -      assert(devinfo->gen >= 8 || type < BRW_REGISTER_TYPE_Q);

It might be good to keep these asserts around, but phrased as something
like assert(gen_has_reg_type(gen, file, type)) or something.

>        return hw_types[type];
>     }
>  }
> @@ -184,9 +181,6 @@ brw_hw_reg_type_to_size(const struct gen_device_info *devinfo,
>           [GEN8_HW_REG_NON_IMM_TYPE_HF] = 2,
>        };
>        assert(type < ARRAY_SIZE(hw_sizes));
> -      assert(devinfo->gen >= 7 ||
> -             (type < GEN7_HW_REG_NON_IMM_TYPE_DF || type == BRW_HW_REG_TYPE_F));
> -      assert(devinfo->gen >= 8 || type <= BRW_HW_REG_TYPE_F);
>        return hw_sizes[type];
>     }
>  }
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
> index fdc30d450c..0ea4c4f1cc 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -403,6 +403,7 @@ void
>  fs_reg::init()
>  {
>     memset(this, 0, sizeof(*this));
> +   type = BRW_REGISTER_TYPE_UD;
>     stride = 1;
>  }
>  
> diff --git a/src/intel/compiler/brw_reg.h b/src/intel/compiler/brw_reg.h
> index 17a51fbd65..48e6fd7b7d 100644
> --- a/src/intel/compiler/brw_reg.h
> +++ b/src/intel/compiler/brw_reg.h
> @@ -203,29 +203,25 @@ brw_mask_for_swizzle(unsigned swz)
>  }
>  
>  enum PACKED brw_reg_type {

And maybe add a comment here like this part of the commit message:

 * The ordering has been chosen so that no enum value is the same as a
 * compatible hardware encoding.

So it's clear that matching some hardware encoding is an anti-goal.

fwiw:

Reviewed-by: Scott D Phillips <scott.d.phillips at intel.com>

> -   BRW_REGISTER_TYPE_UD = 0,
> -   BRW_REGISTER_TYPE_D,
> -   BRW_REGISTER_TYPE_UW,
> -   BRW_REGISTER_TYPE_W,
> +   /** Floating-point types: @{ */
> +   BRW_REGISTER_TYPE_DF,
>     BRW_REGISTER_TYPE_F,
> -
> -   /** Non-immediates only: @{ */
> -   BRW_REGISTER_TYPE_UB,
> -   BRW_REGISTER_TYPE_B,
> -   /** @} */
> -
> -   /** Immediates only: @{ */
> -   BRW_REGISTER_TYPE_UV, /* Gen6+ */
> -   BRW_REGISTER_TYPE_V,
> +   BRW_REGISTER_TYPE_HF,
>     BRW_REGISTER_TYPE_VF,
>     /** @} */
>  
> -   BRW_REGISTER_TYPE_DF, /* Gen7+ (no immediates until Gen8+) */
> -
> -   /* Gen8+ */
> -   BRW_REGISTER_TYPE_HF,
> -   BRW_REGISTER_TYPE_UQ,
> +   /** Integer types: @{ */
>     BRW_REGISTER_TYPE_Q,
> +   BRW_REGISTER_TYPE_UQ,
> +   BRW_REGISTER_TYPE_D,
> +   BRW_REGISTER_TYPE_UD,
> +   BRW_REGISTER_TYPE_W,
> +   BRW_REGISTER_TYPE_UW,
> +   BRW_REGISTER_TYPE_B,
> +   BRW_REGISTER_TYPE_UB,
> +   BRW_REGISTER_TYPE_V,
> +   BRW_REGISTER_TYPE_UV,
> +   /** @} */
>  };
>  
>  unsigned brw_reg_type_to_hw_type(const struct gen_device_info *devinfo,
> diff --git a/src/intel/compiler/brw_vec4.cpp b/src/intel/compiler/brw_vec4.cpp
> index 410922c62b..bf9a271900 100644
> --- a/src/intel/compiler/brw_vec4.cpp
> +++ b/src/intel/compiler/brw_vec4.cpp
> @@ -42,8 +42,8 @@ void
>  src_reg::init()
>  {
>     memset(this, 0, sizeof(*this));
> -
>     this->file = BAD_FILE;
> +   this->type = BRW_REGISTER_TYPE_UD;
>  }
>  
>  src_reg::src_reg(enum brw_reg_file file, int nr, const glsl_type *type)
> @@ -85,6 +85,7 @@ dst_reg::init()
>  {
>     memset(this, 0, sizeof(*this));
>     this->file = BAD_FILE;
> +   this->type = BRW_REGISTER_TYPE_UD;
>     this->writemask = WRITEMASK_XYZW;
>  }
>  
> -- 
> 2.13.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev