From 4ba445e0117b29c31b030feb6e0f421a5ceb03e5 Mon Sep 17 00:00:00 2001 From: Jason Ekstrand Date: Mon, 8 Oct 2018 12:22:35 -0500 Subject: [PATCH] intel: Don't propagate conditional modifiers if a UD source is negated This fixes a bug uncovered by my NIR integer division by constant optimization series. Fixes: 19f9cb72c8b "i965/fs: Add pass to propagate conditional..." Fixes: 627f94b72e0 "i965/vec4: adding vec4_cmod_propagation..." Reviewed-by: Ian Romanick --- src/intel/compiler/brw_fs.cpp | 19 +++++++++++++++++++ src/intel/compiler/brw_ir_fs.h | 1 + src/intel/compiler/brw_ir_vec4.h | 1 + src/intel/compiler/brw_reg.h | 9 +++++++++ src/intel/compiler/brw_vec4.cpp | 20 ++++++++++++++++++++ 5 files changed, 50 insertions(+) diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp index 3f7f2b4c984..23a25fedca5 100644 --- a/src/intel/compiler/brw_fs.cpp +++ b/src/intel/compiler/brw_fs.cpp @@ -395,6 +395,25 @@ fs_inst::can_do_source_mods(const struct gen_device_info *devinfo) return true; } +bool +fs_inst::can_do_cmod() +{ + if (!backend_instruction::can_do_cmod()) + return false; + + /* The accumulator result appears to get used for the conditional modifier + * generation. When negating a UD value, there is a 33rd bit generated for + * the sign in the accumulator value, so now you can't check, for example, + * equality with a 32-bit value. See piglit fs-op-neg-uvec4. + */ + for (unsigned i = 0; i < sources; i++) { + if (type_is_unsigned_int(src[i].type) && src[i].negate) + return false; + } + + return true; +} + bool fs_inst::can_change_types() const { diff --git a/src/intel/compiler/brw_ir_fs.h b/src/intel/compiler/brw_ir_fs.h index 92dad269a34..07e7224e0f8 100644 --- a/src/intel/compiler/brw_ir_fs.h +++ b/src/intel/compiler/brw_ir_fs.h @@ -354,6 +354,7 @@ public: unsigned components_read(unsigned i) const; unsigned size_read(int arg) const; bool can_do_source_mods(const struct gen_device_info *devinfo); + bool can_do_cmod(); bool can_change_types() const; bool has_source_and_destination_hazard() const; diff --git a/src/intel/compiler/brw_ir_vec4.h b/src/intel/compiler/brw_ir_vec4.h index e401d8b4d16..65b1e4f3b53 100644 --- a/src/intel/compiler/brw_ir_vec4.h +++ b/src/intel/compiler/brw_ir_vec4.h @@ -291,6 +291,7 @@ public: int swizzle, int swizzle_mask); void reswizzle(int dst_writemask, int swizzle); bool can_do_source_mods(const struct gen_device_info *devinfo); + bool can_do_cmod(); bool can_do_writemask(const struct gen_device_info *devinfo); bool can_change_types() const; bool has_source_and_destination_hazard() const; diff --git a/src/intel/compiler/brw_reg.h b/src/intel/compiler/brw_reg.h index ac12ab3d2dd..46d66198a1d 100644 --- a/src/intel/compiler/brw_reg.h +++ b/src/intel/compiler/brw_reg.h @@ -376,6 +376,15 @@ brw_int_type(unsigned sz, bool is_signed) } } +static inline bool +type_is_unsigned_int(enum brw_reg_type tp) +{ + return tp == BRW_REGISTER_TYPE_UB || + tp == BRW_REGISTER_TYPE_UW || + tp == BRW_REGISTER_TYPE_UD || + tp == BRW_REGISTER_TYPE_UQ; +} + /** * Construct a brw_reg. * \param file one of the BRW_x_REGISTER_FILE values diff --git a/src/intel/compiler/brw_vec4.cpp b/src/intel/compiler/brw_vec4.cpp index 5a86f30634a..74a4d09fc79 100644 --- a/src/intel/compiler/brw_vec4.cpp +++ b/src/intel/compiler/brw_vec4.cpp @@ -257,6 +257,26 @@ vec4_instruction::can_do_source_mods(const struct gen_device_info *devinfo) return true; } +bool +vec4_instruction::can_do_cmod() +{ + if (!backend_instruction::can_do_cmod()) + return false; + + /* The accumulator result appears to get used for the conditional modifier + * generation. When negating a UD value, there is a 33rd bit generated for + * the sign in the accumulator value, so now you can't check, for example, + * equality with a 32-bit value. See piglit fs-op-neg-uvec4. + */ + for (unsigned i = 0; i < 3; i++) { + if (src[i].file != BAD_FILE && + type_is_unsigned_int(src[i].type) && src[i].negate) + return false; + } + + return true; +} + bool vec4_instruction::can_do_writemask(const struct gen_device_info *devinfo) {