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

[Kenneth Graunke]

On Wednesday, January 21, 2015 03:05:03 PM Matt Turner wrote:
> On Wed, Jan 21, 2015 at 2:52 PM, Kenneth Graunke <kenn...@whitecape.org> 
> wrote:
> > On Wednesday, January 21, 2015 01:02:46 PM Matt Turner wrote:
> >> On Tue, Jan 20, 2015 at 12:11 AM, Kenneth Graunke <kenn...@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 <kenn...@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".
> 
> No. It gives the conditional mod you get by negating both operands of
> the comparison.
> 
> > 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.
> 
> If you review the rest of the series (please do!), you'll see this

It's on my list...

> function used in patch 14/16. The commit message gives an example of
> where we want to use this function, transforming:
> 
>    add(8)       temp   a      b
>    cmp.l.f0(8)  null   -temp  0.0
> 
> into
> 
>    add.ge.f0(8) temp   a      b
> 
> Imagine that was instead of cmp.l.f0, cmp.z.f0:
> 
>    add(8)       temp   a      b
>    cmp.z.f0(8)  null   -temp  0.0
> 
> This is doing f0 = (-(a + b) == 0.0). If I swap Z <-> NZ, I'd get
> 
>    add.nz.f0(8) temp a b
> 
> which would be f0 = ((a + b) != 0.0). In fact, we want the cmp's
> original .z on the add:
> 
>    add.z.f0(8) temp a b
> 
> Perhaps there's a better name we can give the function?

Okay, I see what you're doing now.  It's definitely not what my
brw_negate_cmod function did.  This is why you absolutely need
doxygen comments, ideally with an example.  The functions for
(a cmp1 b) -> !(a cmp1 b) and (a cmp1 b) -> (!a cmp2 !b) look
way too similar at a glance...

brw_invert_cmod is probably fine, with a comment.

signature.asc
Description: This is a digitally signed message part.

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev