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

Alejandro Piñeiro apinheiro at igalia.com
Fri Mar 23 15:17:48 UTC 2018


On 23/03/18 16:13, Ian Romanick wrote:
> On 03/23/2018 01:21 AM, Alejandro Piñeiro wrote:
>> On 22/03/18 19:08, Ian Romanick wrote:
>>> On 03/22/2018 01:12 AM, Alejandro Piñeiro wrote:
>>>> Looks good in general, just a comment below.
>>>>
>>>>
>>>> On 22/03/18 01:58, Ian Romanick 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:
>>>>> +         /* FINISHME: Implement support for these types. */
>>>> Is this missing functionality on purpose or the patch is still a wip? If
>>>> it is the former, perhaps it would be good to explain why it is ok to
>>>> leave that functionality hole.
>>> Basically this means that any optimizations that depend on
>>> negative_equals won't happen.  I didn't implement these paths because,
>>> as far as I can tell, we never generate any code that uses these types.
>>> I was a bit reluctant to implement something that I couldn't test.
>> Hmm, all that reasoning seems to go for a "assert("unreachable")"
>> instead of a FINISHME comment. But I guess that that is somewhat too harsh?
> Hmm... I'm not sure.  They're still valid types.  They're not optimized
> yet because they aren't any current users, but once there are users, we
> should implement the optimization.  An assertion would be a strong reminder.

Well, I think that at this point we are starting to bikeshed without
choosing an option. I'm biased for an assert, but I think that it is not
a big deal leaving just that comment.

So, with or without an assert (as you prefer):
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>


>
>>>>> +         return false;
>>>>> +      default:
>>>>> +         unreachable("not reached");
>>>>> +      }
>>>>> +   } else {
>>>>> +      struct brw_reg tmp = *a;
>>>>> +
>>>>> +      tmp.negate = !tmp.negate;
>>>>> +
>>>>> +      return brw_regs_equal(&tmp, b);
>>>>> +   }
>>>>> +}
>>>>> +
>>>>>  struct brw_indirect {
>>>>>     unsigned addr_subnr:4;
>>>>>     int addr_offset:10;
>>>>> diff --git a/src/intel/compiler/brw_shader.cpp b/src/intel/compiler/brw_shader.cpp
>>>>> index 054962b..9cdf9fc 100644
>>>>> --- a/src/intel/compiler/brw_shader.cpp
>>>>> +++ b/src/intel/compiler/brw_shader.cpp
>>>>> @@ -685,6 +685,12 @@ backend_reg::equals(const backend_reg &r) const
>>>>>  }
>>>>>  
>>>>>  bool
>>>>> +backend_reg::negative_equals(const backend_reg &r) const
>>>>> +{
>>>>> +   return brw_regs_negative_equal(this, &r) && offset == r.offset;
>>>>> +}
>>>>> +
>>>>> +bool
>>>>>  backend_reg::is_zero() const
>>>>>  {
>>>>>     if (file != IMM)
>>>>> diff --git a/src/intel/compiler/brw_shader.h b/src/intel/compiler/brw_shader.h
>>>>> index fd02feb..7d97ddb 100644
>>>>> --- a/src/intel/compiler/brw_shader.h
>>>>> +++ b/src/intel/compiler/brw_shader.h
>>>>> @@ -59,6 +59,7 @@ struct backend_reg : private brw_reg
>>>>>     }
>>>>>  
>>>>>     bool equals(const backend_reg &r) const;
>>>>> +   bool negative_equals(const backend_reg &r) const;
>>>>>  
>>>>>     bool is_zero() const;
>>>>>     bool is_one() const;
>>>>> diff --git a/src/intel/compiler/brw_vec4.cpp b/src/intel/compiler/brw_vec4.cpp
>>>>> index e483814..6680410 100644
>>>>> --- a/src/intel/compiler/brw_vec4.cpp
>>>>> +++ b/src/intel/compiler/brw_vec4.cpp
>>>>> @@ -376,6 +376,13 @@ src_reg::equals(const src_reg &r) const
>>>>>  }
>>>>>  
>>>>>  bool
>>>>> +src_reg::negative_equals(const src_reg &r) const
>>>>> +{
>>>>> +   return this->backend_reg::negative_equals(r) &&
>>>>> +          !reladdr && !r.reladdr;
>>>>> +}
>>>>> +
>>>>> +bool
>>>>>  vec4_visitor::opt_vector_float()
>>>>>  {
>>>>>     bool progress = false;
>



More information about the mesa-dev mailing list