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

[Ian Romanick]

On 03/23/2018 12:17 PM, Chema Casanova wrote:
> 
> 
> On 23/03/18 19:27, Matt Turner wrote:
>> 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.
> 
> According to PRM:
> 
> "For a word, unsigned word, or half-float immediate data, software
> must replicate the same 16-bit immediate value to both the lower word
> and the high word of the 32-bit immediate field in a GEN instruction"
> 
> So if we want to have this already implemented we just need to write for
> W,UW and HF as:
> 
> 	return a->ud == (b->ud ^ 0x80008000);

I don't think the implementation is hard.  Like I said in a reply to
Alejandro, I'm reluctant to add code that I cannot test.  keithp is
often fond of saying, "Any code that isn't tested *is* broken."  The
older I get, the more I agree with that sentiment. :)

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