On 04/09/2015 09:31 AM, Matt Turner wrote:
> On Wed, Apr 8, 2015 at 4:38 PM, Ian Romanick <i...@freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.roman...@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.roman...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
