[Mesa-dev] [PATCH 3/5] i965/vs: Add src_reg::negative_equals method

[Matt Turner]

On Wed, Apr 8, 2015 at 4:38 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 method.  Instead of
> testing that two src_regs are equal to each other, it tests that one is
> the negation of the other.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_ir_vec4.h |  1 +
>  src/mesa/drivers/dri/i965/brw_vec4.cpp  | 43 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_vec4.h b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> index d3bd64d..449795a 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
> @@ -49,6 +49,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/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index ef2fd40..d5286c2 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -328,6 +328,49 @@ src_reg::equals(const src_reg &r) const
>  }
>
>  bool
> +src_reg::negative_equals(const src_reg &r) const
> +{
> +   if (file != r.file)
> +      return false;
> +
> +   if (file == IMM) {
> +      if (!(reg == r.reg &&
> +            reg_offset == r.reg_offset &&
> +            type == r.type &&
> +            negate == r.negate &&
> +            abs == r.abs &&
> +            swizzle == r.swizzle &&
> +            !reladdr && !r.reladdr))

I would have applied DeMorgan's Law and made this a series of ORs.

> +         return false;
> +
> +      switch (fixed_hw_reg.type) {

I don't think we keep fixed_hw_reg.type in sync with src_reg's type.
(Another reason to move towards a brw_reg-based register model)

Looking at the constructors for immediate, this appears to be the
case. Just changing this to switch (type) should do the trick.

> +      case BRW_REGISTER_TYPE_F:
> +         return memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
> +                       sizeof(fixed_hw_reg) - sizeof(fixed_hw_reg.dw1)) == 0 &&
> +                fixed_hw_reg.dw1.f == -r.fixed_hw_reg.dw1.f;

I don't think you need the memcmp. For immediates, we really just use
the fixed_hw_reg for the immediate storage itself and none of the
other metadata.

> +
> +      case BRW_REGISTER_TYPE_D:
> +         return memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
> +                       sizeof(fixed_hw_reg) - sizeof(fixed_hw_reg.dw1)) == 0 &&
> +                fixed_hw_reg.dw1.d == -r.fixed_hw_reg.dw1.d;
> +
> +      default:
> +         return false;
> +      }
> +   } else {
> +      return reg == r.reg &&
> +             reg_offset == r.reg_offset &&
> +             type == r.type &&
> +             negate != r.negate &&
> +             abs == r.abs &&
> +             swizzle == r.swizzle &&
> +             !reladdr && !r.reladdr &&
> +             memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
> +                    sizeof(fixed_hw_reg)) == 0;

Indent these to match reg == ...

I think the function does what it advertises. I just don't think that
happens to be the thing you really want. See next patch's reply.