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

[Jason Ekstrand]

On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga <itoral at igalia.com>
wrote:

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


>     return imm;
>  }
>
> --
> 2.14.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180430/e344b4dc/attachment.html>