[Mesa-dev] [PATCH 06/16] i965: Add a brw_invert_cmod() function.

[Kenneth Graunke]

On Wednesday, January 21, 2015 01:02:46 PM Matt Turner wrote:
> On Tue, Jan 20, 2015 at 12:11 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> > On Monday, January 19, 2015 03:31:05 PM Matt Turner wrote:
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_eu.c | 22 ++++++++++++++++++++++
> >>  src/mesa/drivers/dri/i965/brw_eu.h |  1 +
> >>  2 files changed, 23 insertions(+)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_eu.c b/src/mesa/drivers/dri/i965/brw_eu.c
> >> index 9905972..9977eed 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_eu.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_eu.c
> >> @@ -88,6 +88,28 @@ brw_swap_cmod(uint32_t cmod)
> >>     }
> >>  }
> >>
> >> +/* Returns the corresponding inverted conditional mod. */
> >> +enum brw_conditional_mod
> >> +brw_invert_cmod(enum brw_conditional_mod cmod)
> >> +{
> >> +   switch (cmod) {
> >> +   case BRW_CONDITIONAL_Z:
> >> +      return BRW_CONDITIONAL_NZ;
> >> +   case BRW_CONDITIONAL_NZ:
> >> +      return BRW_CONDITIONAL_Z;
> >> +   case BRW_CONDITIONAL_G:
> >> +      return BRW_CONDITIONAL_LE;
> >> +   case BRW_CONDITIONAL_GE:
> >> +      return BRW_CONDITIONAL_L;
> >> +   case BRW_CONDITIONAL_L:
> >> +      return BRW_CONDITIONAL_GE;
> >> +   case BRW_CONDITIONAL_LE:
> >> +      return BRW_CONDITIONAL_G;
> >> +   default:
> >> +      return BRW_CONDITIONAL_NONE;
> >> +   }
> >> +}
> >
> > Heh, I thought this looked familiar...apparently I wrote one too :)
> > http://lists.freedesktop.org/archives/mesa-dev/2014-August/066127.html
> >
> > I wasn't sure whether "invert" meant "flip direction" or "negate condition"
> > until I read the code.  How about calling it brw_negate_cmod instead?
> >
> > /* Returns a conditional modifier that negates the condition. */
> > enum brw_conditional_mod
> > brw_negate_cmod(uint32_t cmod)
> > {
> >    ...
> > }
> >
> > Either way is fine.
> > Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> Okay, I changed the name, and in the process of thinking about that
> and a comment Jason made I realized that part of this function was
> wrong. When you negate, you don't want to swap Z <-> NZ. I've changed
> the function accordingly:
> 
> +   switch (cmod) {
> +   case BRW_CONDITIONAL_Z:
> +   case BRW_CONDITIONAL_NZ:
> +      return cmod;
> [snip]

What?  This function takes a comparison that produces "A" and gives you
a new one that produces "!A".  You absolutely have to change == to != (and
vice versa), or the function is just plain broken.  You had it right the
first time.

brw_swap_cmod doesn't do that, because it takes `X CMP Y` => A and
returns a new comparison such that `Y CMP' X` >= A.  In other words, if
X < Y, then Y > X.  But if X == Y, then Y == X.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150121/5efdfeee/attachment.sig>