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

[Chema Casanova]

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>> wrote:
> 
>     From: Jose Maria Casanova Crespo <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.

>         return imm;
>      }
>      
>     -- 
>     2.14.1
> 
>