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.roman...@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.roman...@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.
>> + 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;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
