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

[Ian Romanick]

On 01/21/2015 03:05 PM, Matt Turner wrote:
> 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.

Which is the same as if you commute the arguments, right?  Maybe
brw_cmod_for_commuted_args or brw_cmod_for_negated_args is a better
name.  Based on this discussion, I think the function pre-comment is
also a little misleading.  It sounds like it returns the !A cmod.

>> 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?
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>