[Mesa-dev] [PATCH 2/5] i965: Add function to negate immediates.

[Matt Turner]

On Fri, Jan 30, 2015 at 4:02 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Fri, Jan 30, 2015 at 3:54 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 37
>> ++++++++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_shader.h   |  1 +
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index c393bfc..ff2edf3 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -620,6 +620,43 @@ brw_saturate_immediate(enum brw_reg_type type, struct
>> brw_reg *reg)
>>     return false;
>>  }
>>
>> +bool
>> +brw_negate_immediate(enum brw_reg_type type, struct brw_reg *reg)
>> +{
>> +   switch (type) {
>> +   case BRW_REGISTER_TYPE_UD:
>> +   case BRW_REGISTER_TYPE_D:
>> +      reg->dw1.d = -reg->dw1.d;
>> +      return true;
>> +   case BRW_REGISTER_TYPE_UW:
>> +      reg->dw1.d = -(uint16_t)reg->dw1.ud;
>
>
> Do we really want to negate UW and UD sources?  If so, shouldn't we change
> the register type to signed or something?  Maybe there's something I don't
> know about the arch, but this seems fishy.

I think it's okay. The source language lets you do it and the hardware
lets you do it. I don't think D vs UD (or W vs UW) changes the
operation the negation modifier performs (maybe except for the case of
using it in conjunction with conditional mod).

In any case, it doesn't catch anything in shader-db, so I don't mind
just returning false for UD and UW.

> Same comment applies on the next patch with abs().

Surely an abs source modifier on an unsigned source is a no-op. Again,
no changes in shader-db because of allowing it though, so I don't mind
just returning false.

> Other than that, everything in this series looks fine.

Thanks. I'll slap your R-b on it.