[Mesa-dev] [PATCH 1/6] i965: Add negative_equals methods

[Matt Turner]

On Wed, Mar 21, 2018 at 5:58 PM, Ian Romanick <idr at freedesktop.org> wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This method is similar to the existing ::equals methods.  Instead of
> testing that two src_regs are equal to each other, it tests that one is
> the negation of the other.
>
> v2: Simplify various checks based on suggestions from Matt.  Use
> src_reg::type instead of fixed_hw_reg.type in a check.  Also suggested
> by Matt.
>
> v3: Rebase on 3 years.  Fix some problems with negative_equals with VF
> constants.  Add fs_reg::negative_equals.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/intel/compiler/brw_fs.cpp     |  7 ++++++
>  src/intel/compiler/brw_ir_fs.h    |  1 +
>  src/intel/compiler/brw_ir_vec4.h  |  1 +
>  src/intel/compiler/brw_reg.h      | 46 +++++++++++++++++++++++++++++++++++++++
>  src/intel/compiler/brw_shader.cpp |  6 +++++
>  src/intel/compiler/brw_shader.h   |  1 +
>  src/intel/compiler/brw_vec4.cpp   |  7 ++++++
>  7 files changed, 69 insertions(+)
>
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
> index 6eea532..3d454c3 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -454,6 +454,13 @@ fs_reg::equals(const fs_reg &r) const
>  }
>
>  bool
> +fs_reg::negative_equals(const fs_reg &r) const
> +{
> +   return (this->backend_reg::negative_equals(r) &&
> +           stride == r.stride);
> +}
> +
> +bool
>  fs_reg::is_contiguous() const
>  {
>     return stride == 1;
> diff --git a/src/intel/compiler/brw_ir_fs.h b/src/intel/compiler/brw_ir_fs.h
> index 54797ff..f06a33c 100644
> --- a/src/intel/compiler/brw_ir_fs.h
> +++ b/src/intel/compiler/brw_ir_fs.h
> @@ -41,6 +41,7 @@ public:
>     fs_reg(enum brw_reg_file file, int nr, enum brw_reg_type type);
>
>     bool equals(const fs_reg &r) const;
> +   bool negative_equals(const fs_reg &r) const;
>     bool is_contiguous() const;
>
>     /**
> diff --git a/src/intel/compiler/brw_ir_vec4.h b/src/intel/compiler/brw_ir_vec4.h
> index cbaff2f..95c5119 100644
> --- a/src/intel/compiler/brw_ir_vec4.h
> +++ b/src/intel/compiler/brw_ir_vec4.h
> @@ -43,6 +43,7 @@ public:
>     src_reg(struct ::brw_reg reg);
>
>     bool equals(const src_reg &r) const;
> +   bool negative_equals(const src_reg &r) const;
>
>     src_reg(class vec4_visitor *v, const struct glsl_type *type);
>     src_reg(class vec4_visitor *v, const struct glsl_type *type, int size);
> diff --git a/src/intel/compiler/brw_reg.h b/src/intel/compiler/brw_reg.h
> index 7ad144b..732bddf 100644
> --- a/src/intel/compiler/brw_reg.h
> +++ b/src/intel/compiler/brw_reg.h
> @@ -255,6 +255,52 @@ brw_regs_equal(const struct brw_reg *a, const struct brw_reg *b)
>     return a->bits == b->bits && (df ? a->u64 == b->u64 : a->ud == b->ud);
>  }
>
> +static inline bool
> +brw_regs_negative_equal(const struct brw_reg *a, const struct brw_reg *b)
> +{
> +   if (a->file == IMM) {
> +      if (a->bits != b->bits)
> +         return false;
> +
> +      switch (a->type) {
> +      case BRW_REGISTER_TYPE_UQ:
> +      case BRW_REGISTER_TYPE_Q:
> +         return a->d64 == -b->d64;
> +      case BRW_REGISTER_TYPE_DF:
> +         return a->df == -b->df;
> +      case BRW_REGISTER_TYPE_UD:
> +      case BRW_REGISTER_TYPE_D:
> +         return a->d == -b->d;
> +      case BRW_REGISTER_TYPE_F:
> +         return a->f == -b->f;
> +      case BRW_REGISTER_TYPE_VF:
> +         /* It is tempting to treat 0 as a negation of 0 (and -0 as a negation
> +          * of -0).  There are occasions where 0 or -0 is used and the exact
> +          * bit pattern is desired.  At the very least, changing this to allow
> +          * 0 as a negation of 0 causes some fp64 tests to fail on IVB.
> +          */
> +         return a->ud == (b->ud ^ 0x80808080);
> +      case BRW_REGISTER_TYPE_UW:
> +      case BRW_REGISTER_TYPE_W:
> +      case BRW_REGISTER_TYPE_UV:
> +      case BRW_REGISTER_TYPE_V:
> +      case BRW_REGISTER_TYPE_HF:
> +      case BRW_REGISTER_TYPE_UB:
> +      case BRW_REGISTER_TYPE_B:

There are no B/UB immediates, so you can move these to default. In
fact, I'd get rid of the default so we'll get a warning if there are
unhandled types. Probably the only one not already in the list is NF,
which should also be unreachable.

Returning false for unimplemented types seems fine. Immediates of
those types are sufficiently rare that I don't expect this to catch
anything, and in the rare occurrence that it does I wouldn't want the
compiler to assert fail or do something undefined. Really I only
expect HF to ever get hit, and only after we actually start using it.

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