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

[Ian Romanick]

On 10/09/2018 09:00 AM, 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>
> ---
>  .../compiler/brw_fs_cmod_propagation.cpp      | 25 ++++++++++++++++---
>  src/intel/compiler/brw_reg.h                  |  9 +++++++
>  .../compiler/brw_vec4_cmod_propagation.cpp    | 24 ++++++++++++++++--
>  3 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp b/src/intel/compiler/brw_fs_cmod_propagation.cpp
> index 5fb522f810f..4fdd04a9983 100644
> --- a/src/intel/compiler/brw_fs_cmod_propagation.cpp
> +++ b/src/intel/compiler/brw_fs_cmod_propagation.cpp
> @@ -25,6 +25,25 @@
>  #include "brw_cfg.h"
>  #include "brw_eu.h"
>  
> +static bool
> +can_add_cmod_to_inst(const fs_inst *inst)
> +{
> +   if (!inst->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 < inst->sources; i++) {
> +      if (type_is_unsigned_int(inst->src[i].type) && inst->src[i].negate)
> +         return false;
> +   }
> +
> +   return true;
> +}
> +

This probably should go after the @file header comment. :)

Also... it looks like this patch replaces every caller of
::can_do_cmod() with can_add_cmod_to_inst.  Maybe just change
can_do_cmod?  If you do that, I'd support changing the name to
can_add_cmod_to_inst.

>  /** @file brw_fs_cmod_propagation.cpp
>   *
>   * Implements a pass that propagates the conditional modifier from a CMP x 0.0
> @@ -91,7 +110,7 @@ cmod_propagate_cmp_to_add(const gen_device_info *devinfo, bblock_t *block,
>              negate ? brw_swap_cmod(inst->conditional_mod)
>              : inst->conditional_mod;
>  
> -         if (scan_inst->can_do_cmod() &&
> +         if (can_add_cmod_to_inst(scan_inst) &&
>               ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) ||
>                scan_inst->conditional_mod == cond)) {
>              scan_inst->conditional_mod = cond;
> @@ -146,7 +165,7 @@ cmod_propagate_not(const gen_device_info *devinfo, bblock_t *block,
>               scan_inst->exec_size != inst->exec_size)
>              break;
>  
> -         if (scan_inst->can_do_cmod() &&
> +         if (can_add_cmod_to_inst(scan_inst) &&
>               ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) ||
>                scan_inst->conditional_mod == cond)) {
>              scan_inst->conditional_mod = cond;
> @@ -316,7 +335,7 @@ opt_cmod_propagation_local(const gen_device_info *devinfo, bblock_t *block)
>                 inst->src[0].negate ? brw_swap_cmod(inst->conditional_mod)
>                                     : inst->conditional_mod;
>  
> -            if (scan_inst->can_do_cmod() &&
> +            if (can_add_cmod_to_inst(scan_inst) &&
>                  ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) ||
>                   scan_inst->conditional_mod == cond)) {
>                 scan_inst->conditional_mod = cond;
> 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_cmod_propagation.cpp b/src/intel/compiler/brw_vec4_cmod_propagation.cpp
> index a1d46dc8dca..392e283d1f8 100644
> --- a/src/intel/compiler/brw_vec4_cmod_propagation.cpp
> +++ b/src/intel/compiler/brw_vec4_cmod_propagation.cpp
> @@ -46,6 +46,26 @@ writemasks_incompatible(const vec4_instruction *earlier,
>            (later->dst.writemask & ~earlier->dst.writemask) != 0;
>  }
>  
> +static bool
> +can_add_cmod_to_inst(const vec4_instruction *inst)
> +{
> +   if (!inst->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 (inst->src[i].file != BAD_FILE &&
> +          type_is_unsigned_int(inst->src[i].type) && inst->src[i].negate)
> +         return false;
> +   }
> +
> +   return true;
> +}
> +
>  static bool
>  opt_cmod_propagation_local(bblock_t *block)
>  {
> @@ -132,7 +152,7 @@ opt_cmod_propagation_local(bblock_t *block)
>                 negate ? brw_swap_cmod(inst->conditional_mod)
>                        : inst->conditional_mod;
>  
> -            if (scan_inst->can_do_cmod() &&
> +            if (can_add_cmod_to_inst(scan_inst) &&
>                  ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) ||
>                   scan_inst->conditional_mod == cond)) {
>                 scan_inst->conditional_mod = cond;
> @@ -229,7 +249,7 @@ opt_cmod_propagation_local(bblock_t *block)
>                 inst->src[0].negate ? brw_swap_cmod(inst->conditional_mod)
>                                     : inst->conditional_mod;
>  
> -            if (scan_inst->can_do_cmod() &&
> +            if (can_add_cmod_to_inst(scan_inst) &&
>                  ((!read_flag && scan_inst->conditional_mod == BRW_CONDITIONAL_NONE) ||
>                   scan_inst->conditional_mod == cond)) {
>                 scan_inst->conditional_mod = cond;
>