[Mesa-dev] [PATCH 1/6] i965: Add negative_equals methods
Ian Romanick
idr at freedesktop.org
Fri Mar 23 15:13:09 UTC 2018
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.
>>>> + 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