[Mesa-dev] [PATCH] i965: Fix undefined df bits in brw_reg comparisons.

[Matt Turner]

On Fri, May 13, 2016 at 5:44 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Commit 5310bca024f77da40ea6f4c275455f9cb0528f9e added a new "double df"
> field to the brw_reg struct, adding an extra 4 bytes of data that isn't
> usually initialized (or may contain irrelevant garbage if the struct is
> mutated).  This means that it's no longer safe to memcmp().
>
> Instead, add a brw_regs_equal() function which ignores the extra df bits
> unless they matter.  To keep the implementation cheap, we wrap the first
> set of fields in a union/struct so that we can use a single DWord
> comparison.
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   |  2 +-
>  src/mesa/drivers/dri/i965/brw_reg.h              | 27 +++++++++++++++++-------
>  src/mesa/drivers/dri/i965/brw_shader.cpp         |  2 +-
>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp |  2 +-
>  4 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> index 4f6f3a3..3b50a82 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> @@ -1010,7 +1010,7 @@ fs_generator::generate_tex(fs_inst *inst, struct brw_reg dst, struct brw_reg src
>        brw_set_default_mask_control(p, BRW_MASK_DISABLE);
>        brw_set_default_access_mode(p, BRW_ALIGN_1);
>
> -      if (memcmp(&surface_reg, &sampler_reg, sizeof(surface_reg)) == 0) {
> +      if (brw_regs_equal(&surface_reg, &sampler_reg)) {
>           brw_MUL(p, addr, sampler_reg, brw_imm_uw(0x101));
>        } else {
>           brw_SHL(p, addr, sampler_reg, brw_imm_ud(8));
> diff --git a/src/mesa/drivers/dri/i965/brw_reg.h b/src/mesa/drivers/dri/i965/brw_reg.h
> index 6d51623..71e1024 100644
> --- a/src/mesa/drivers/dri/i965/brw_reg.h
> +++ b/src/mesa/drivers/dri/i965/brw_reg.h
> @@ -234,14 +234,19 @@ uint32_t brw_swizzle_immediate(enum brw_reg_type type, uint32_t x, unsigned swz)
>   * or "structure of array" form:
>   */
>  struct brw_reg {
> -   enum brw_reg_type type:4;
> -   enum brw_reg_file file:3;      /* :2 hardware format */
> -   unsigned negate:1;             /* source only */
> -   unsigned abs:1;                /* source only */
> -   unsigned address_mode:1;       /* relative addressing, hopefully! */
> -   unsigned pad0:1;
> -   unsigned subnr:5;              /* :1 in align16 */
> -   unsigned nr:16;
> +   union {
> +      struct {
> +         enum brw_reg_type type:4;
> +         enum brw_reg_file file:3;      /* :2 hardware format */
> +         unsigned negate:1;             /* source only */
> +         unsigned abs:1;                /* source only */
> +         unsigned address_mode:1;       /* relative addressing, hopefully! */
> +         unsigned pad0:1;
> +         unsigned subnr:5;              /* :1 in align16 */
> +         unsigned nr:16;
> +      };
> +      uint32_t bits;
> +   };
>
>     union {
>        struct {
> @@ -261,6 +266,12 @@ struct brw_reg {
>     };
>  };
>
> +static inline bool
> +brw_regs_equal(const struct brw_reg *a, const struct brw_reg *b)
> +{
> +   const bool df = a->type == BRW_REGISTER_TYPE_DF && a->file == IMM;
> +   return a->bits == b->bits && (df ? a->df == b->df : a->ud == b->ud);

We should add a u64 field and compare that, since comparing df == df
won't tell the whole story regarding signed zeros and NaN.

But that doesn't need to happen in this patch.

Reviewed-by: Matt Turner <mattst88 at gmail.com>