[Mesa-dev] [PATCH v2] intel: Don't propagate conditional modifiers if a UD source is negated

[Ian Romanick]

Works for me.

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

On 10/09/2018 05:19 PM, Jason Ekstrand wrote:
> 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..."
> Cc: Matt Turner <mattst88 at gmail.com>
> ---
>  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)
>  {
>