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.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.
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? > >>> + 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