[Mesa-dev] [PATCH] i965/vec4: copy abs/negate modifiers on src/dst_reg conversion constructors

[Jason Ekstrand]

On Wed, Sep 30, 2015 at 11:03 AM, Alejandro Piñeiro
<apinheiro at igalia.com> wrote:
>
>
> On 30/09/15 19:38, Jason Ekstrand wrote:
>> I'm not sure if this makes sense.  I can see how it would be useful
>> (less information lost when going src_reg -> dst_reg -> src_reg).
>> However, it seems wrong to me to assume that dst_reg.abs or
>> dst_reg.negate means anything useful.
>
> Hmm, true. FWIW, this commit didn't solve any bug on master, but one I
> had on my wip code, while porting some code from
> test_fs_cmod_propagation to a vec4 equivalent.
>
> The original code is this:
>    bld.ADD(dest, src0, src1);
>    dest.negate = true;
>    bld.CMP(bld.null_reg_f(), dest, zero, BRW_CONDITIONAL_GE);
>
> That being translated without too much thinking to vec4 is something like:
>
>    bld.ADD(dest, src0, src1);
>    dest.negate = true;
>    bld.CMP(bld.null_reg_f(), src_reg(dest), zero, BRW_CONDITIONAL_GE);
>
> That fails as src_reg doesn't copy the negate. I got misled by the
> original "dest.negate = true", without realizing that doesn't makes too
> much sense. It was just set to be used as a source on the following
> comparison.
>
> At this point the only advantage of using this patch is avoiding some
> lines with auxiliar vars like this:
>    bld.ADD(dest, src0, src1);
>    src_reg tmp = src_reg(dest);
>    tmp.negate = true;
>    bld.CMP(bld.null_reg_f(), tmp, zero, BRW_CONDITIONAL_GE);
>
> So I think that it would be better to forget this patch. Sorry for the
> noise.

That's ok.  For what it's worth, this one caught me as well when
porting the boolean resolve code to vec4.
--Jason

>>  Matt?
>> --Jason
>>
>> On Wed, Sep 30, 2015 at 10:32 AM, Alejandro Piñeiro
>> <apinheiro at igalia.com> wrote:
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>> index c61b385..121e698 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>>> @@ -139,6 +139,8 @@ src_reg::src_reg(const dst_reg &reg)
>>>     this->reladdr = reg.reladdr;
>>>     this->fixed_hw_reg = reg.fixed_hw_reg;
>>>     this->swizzle = brw_swizzle_for_mask(reg.writemask);
>>> +   this->negate = reg.negate;
>>> +   this->abs = reg.abs;
>>>  }
>>>
>>>  void
>>> @@ -204,6 +206,8 @@ dst_reg::dst_reg(const src_reg &reg)
>>>     this->writemask = brw_mask_for_swizzle(reg.swizzle);
>>>     this->reladdr = reg.reladdr;
>>>     this->fixed_hw_reg = reg.fixed_hw_reg;
>>> +   this->abs = reg.abs;
>>> +   this->negate = reg.negate;
>>>  }
>>>
>>>  bool
>>> --
>>> 2.1.4
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> --
> Alejandro Piñeiro (apinheiro at igalia.com)
>