[Mesa-dev] [PATCH v2 06/18] intel/compiler: fix brw_imm_w for negative 16-bit integers

[Jason Ekstrand]

On Wed, May 2, 2018 at 8:19 AM, Chema Casanova <jmcasanova at igalia.com>
wrote:

>
>
> El 01/05/18 a las 01:22, Jason Ekstrand escribió:
> > On Mon, Apr 30, 2018 at 3:53 PM, Chema Casanova <jmcasanova at igalia.com
> > <mailto:jmcasanova at igalia.com>> wrote:
> >
> >
> >
> >     On 30/04/18 23:12, Jason Ekstrand wrote:
> >     > On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga <
> itoral at igalia.com <mailto:itoral at igalia.com>
> >     > <mailto:itoral at igalia.com <mailto:itoral at igalia.com>>> wrote:
> >     >
> >     >     From: Jose Maria Casanova Crespo <jmcasanova at igalia.com
> <mailto:jmcasanova at igalia.com>
> >     >     <mailto:jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>>>
> >     >
> >     >     16-bit immediates need to replicate the 16-bit immediate value
> >     >     in both words of the 32-bit value. This needs to be careful
> >     >     to avoid sign-extension, which the previous implementation was
> >     >     not handling properly.
> >     >
> >     >     For example, with the previous implementation, storing the
> value
> >     >     -3 would generate imm.d = 0xfffffffd due to signed integer sign
> >     >     extension, which is not correct. Instead, we should cast to
> >     >     unsigned, which gives us the correct result: imm.ud =
> 0xfffdfffd.
> >     >
> >     >     We only had a couple of cases hitting this path in the driver
> >     >     until now, one with value -1, which would work since all bits
> are
> >     >     one in this case, and another with value -2 in brw_clip_tri(),
> >     >     which would hit the aforementioned issue (this case only
> affects
> >     >     gen4 although we are not aware of whether this was causing an
> >     >     actual bug somewhere).
> >     >     ---
> >     >      src/intel/compiler/brw_reg.h | 2 +-
> >     >      1 file changed, 1 insertion(+), 1 deletion(-)
> >     >
> >     >     diff --git a/src/intel/compiler/brw_reg.h
> >     b/src/intel/compiler/brw_reg.h
> >     >     index dff9b970b2..0084a78af6 100644
> >     >     --- a/src/intel/compiler/brw_reg.h
> >     >     +++ b/src/intel/compiler/brw_reg.h
> >     >     @@ -705,7 +705,7 @@ static inline struct brw_reg
> >     >      brw_imm_w(int16_t w)
> >     >      {
> >     >         struct brw_reg imm = brw_imm_reg(BRW_REGISTER_TYPE_W);
> >     >     -   imm.d = w | (w << 16);
> >     >     +   imm.ud = (uint16_t)w | ((uint16_t)w << 16);
> >
> >     > Uh... Is this cast right?  Doing a << 16 on a 16-bit data type
> should
> >     > yield undefined results.  I think you want a (uint32_t) cast.
> >
> >     In my test code it was working at least with GCC, I think it is
> because
> >     at the end we have an integer promotion for any type with lower rank
> >     than int.
> >
> >     "Formally, the rule says (C11 6.3.1.1):
> >
> >         If an int can represent all values of the original type (as
> >     restricted by the width, for a bit-field), the value is converted to
> an
> >     int; otherwise, it is converted to an unsigned int. These are called
> the
> >     integer promotions."
> >
> >     But I agree that is clearer if we just use (uint32_t).
> >     I can change also the brw_imm_uw case that has the same issue.
> >
> >
> > Yeah, best to make it clear. :-)
>
> I was wrong, we can't just replace (uint16_t) cast by (uint32_t) because
> the cast from signed short to uint32_t implies sign extension, because
> it seems that sign extensions is done if source is signed and not in
> destination type.
>
> So for example, being w = -2  (0xfffe).
>
> imm.ud = (uint32_t)w | (uint32_t)w << 16;
>
> becomes: 0xfffffffe
>
> So the alternatives I figure out with the correct result are.
>
> imm.ud = (uint32_t) w & 0xffff | (uint32_t)w << 16;
>
> Or:
>
> uint16_t value = w;
> imm.ud = (uint32_t)value | (uint32_t)value << 16;
>
> Or something like:
>
> imm.ud = (uint32_t)(uint16_t)w | ((uint32_t)(uint16_t)w << 16);
>

I think I like this one only you can drop the first (uint32_t) and I don't
think  you need the extra parens on the right.

Honestly, I think I'd probably be ok with the original version too now that
I understand better what's going on.  Either way,

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180502/bb0718d6/attachment.html>