[Mesa-dev] [PATCH 3/5] i965/vs: Add src_reg::negative_equals method

[Ian Romanick]

On 04/09/2015 09:31 AM, Matt Turner wrote:
> On Wed, Apr 8, 2015 at 4:38 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> This method is similar to the existing ::equals method.  Instead of
>> testing that two src_regs are equal to each other, it tests that one is
>> the negation of the other.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_ir_vec4.h |  1 +
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp  | 43 +++++++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_ir_vec4.h b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
>> index d3bd64d..449795a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
>> +++ b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
>> @@ -49,6 +49,7 @@ public:
>>     src_reg(struct brw_reg reg);
>>
>>     bool equals(const src_reg &r) const;
>> +   bool negative_equals(const src_reg &r) const;
>>
>>     src_reg(class vec4_visitor *v, const struct glsl_type *type);
>>     src_reg(class vec4_visitor *v, const struct glsl_type *type, int size);
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> index ef2fd40..d5286c2 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> @@ -328,6 +328,49 @@ src_reg::equals(const src_reg &r) const
>>  }
>>
>>  bool
>> +src_reg::negative_equals(const src_reg &r) const
>> +{
>> +   if (file != r.file)
>> +      return false;
>> +
>> +   if (file == IMM) {
>> +      if (!(reg == r.reg &&
>> +            reg_offset == r.reg_offset &&
>> +            type == r.type &&
>> +            negate == r.negate &&
>> +            abs == r.abs &&
>> +            swizzle == r.swizzle &&
>> +            !reladdr && !r.reladdr))
> 
> I would have applied DeMorgan's Law and made this a series of ORs.

I used copy-and-paste w/minimal changes. :)  I can change it easily enough.

>> +         return false;
>> +
>> +      switch (fixed_hw_reg.type) {
> 
> I don't think we keep fixed_hw_reg.type in sync with src_reg's type.
> (Another reason to move towards a brw_reg-based register model)

Yes... that aspect was pretty confusing.  It looked like the same
information was tracked in several places just for fun.  I don't know
whether it's good news or bad news, but jenkins was still happy with
this change.

> Looking at the constructors for immediate, this appears to be the
> case. Just changing this to switch (type) should do the trick.
> 
>> +      case BRW_REGISTER_TYPE_F:
>> +         return memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
>> +                       sizeof(fixed_hw_reg) - sizeof(fixed_hw_reg.dw1)) == 0 &&
>> +                fixed_hw_reg.dw1.f == -r.fixed_hw_reg.dw1.f;
> 
> I don't think you need the memcmp. For immediates, we really just use
> the fixed_hw_reg for the immediate storage itself and none of the
> other metadata.

Okay.  That wasn't 100% clear in the rest of the code.  I was also
basing this on the src_reg::equals method which does a memcmp of the
whole thing.

Note that this doesn't handle VF constants.  I assume I just need to add
a BRW_REGISTER_TYPE_VF case and compare like

    swizzle == r.swizzle &&
    fixed_hw_reg.dw1.u == (r.fixed_hw_reg.dw1.ud ^ 0x80808080)

Yeah?  I'll add that as a follow-on patch.

Also... are BRW_REGISTER_TYPE_V constants something I need to worry about?

>> +
>> +      case BRW_REGISTER_TYPE_D:
>> +         return memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
>> +                       sizeof(fixed_hw_reg) - sizeof(fixed_hw_reg.dw1)) == 0 &&
>> +                fixed_hw_reg.dw1.d == -r.fixed_hw_reg.dw1.d;
>> +
>> +      default:
>> +         return false;
>> +      }
>> +   } else {
>> +      return reg == r.reg &&
>> +             reg_offset == r.reg_offset &&
>> +             type == r.type &&
>> +             negate != r.negate &&
>> +             abs == r.abs &&
>> +             swizzle == r.swizzle &&
>> +             !reladdr && !r.reladdr &&
>> +             memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
>> +                    sizeof(fixed_hw_reg)) == 0;
> 
> Indent these to match reg == ...

Wait... I'd swear on a previous patch you wanted something indented like
this, and I commented that emacs is annoying for not being able to do
that by default.  The default emacs indentation would be:

      return reg == r.reg &&
         reg_offset == r.reg_offset &&
         type == r.type &&
         negate != r.negate &&
         abs == r.abs &&
         swizzle == r.swizzle &&
         !reladdr && !r.reladdr &&
         memcmp(&fixed_hw_reg, &r.fixed_hw_reg,
                sizeof(fixed_hw_reg)) == 0;

Is that what you wanted, or did you mean something else?

This block was also a copy-and-paste from src_reg::equals.  If that
indentation is wrong, I can submit a clean-up patch for that too... just
to save the next copy-and-paster. :)

> I think the function does what it advertises. I just don't think that
> happens to be the thing you really want. See next patch's reply.