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

[Alejandro Piñeiro]

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.

>  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)