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

[Matt Turner]

On Wed, Jan 21, 2015 at 2:52 PM, Kenneth Graunke <kenneth at 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 <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".

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
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?