[Mesa-dev] [PATCH] intel: Don't propagate conditional modifiers if a UD source is negated
Ian Romanick
idr at freedesktop.org
Tue Oct 9 23:34:56 UTC 2018
On 10/09/2018 10:03 AM, Jason Ekstrand wrote:
> On Tue, Oct 9, 2018 at 11:24 AM Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> 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 <mailto: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. :)
>
>
> Yeah, probably. I'll fix that.
>
>
> 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.
>
>
> It does and that was my first attempt. Unfortunately, we need access to
> the sources for this and we don't have access in backend_instruction so
> this has to be done per-back-end. I thought about changing can_do_cmod
> to just take an opcode and not an instruction to remove the implication
> that it's somehow comprehensive.
Oh bother. :( I'd support that change to can_do_cmod.
> > /** @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;
> >
>
More information about the mesa-dev
mailing list