Re: [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 
wrote:

>
>
> El 01/05/18 a las 01:22, Jason Ekstrand escribió:
> > On Mon, Apr 30, 2018 at 3:53 PM, Chema Casanova  > > wrote:
> >
> >
> >
> > On 30/04/18 23:12, Jason Ekstrand wrote:
> > > On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga <
> ito...@igalia.com 
> > > >> wrote:
> > >
> > > From: Jose Maria Casanova Crespo  
> > > >>
> > >
> > > 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 = 0xfffd 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: 0xfffe
>
> So the alternatives I figure out with the correct result are.
>
> imm.ud = (uint32_t) w & 0x | (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 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Chema Casanova]

El 01/05/18 a las 01:22, Jason Ekstrand escribió:
> On Mon, Apr 30, 2018 at 3:53 PM, Chema Casanova  > wrote:
> 
> 
> 
> On 30/04/18 23:12, Jason Ekstrand wrote:
> > On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga  
> > >> wrote:
> > 
> >     From: Jose Maria Casanova Crespo  
> >     >>
> >
> >     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 = 0xfffd 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: 0xfffe

So the alternatives I figure out with the correct result are.

imm.ud = (uint32_t) w & 0x | (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);

Any preference?

Chema
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 3:53 PM, Chema Casanova 
wrote:

>
>
> On 30/04/18 23:12, Jason Ekstrand wrote:
> > On Mon, Apr 30, 2018 at 7:18 AM, Iago Toral Quiroga  > > wrote:
> >
> > From: Jose Maria Casanova Crespo  > >
> >
> > 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 = 0xfffd 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. :-)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  > wrote:
> 
> From: Jose Maria Casanova Crespo  >
> 
> 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 = 0xfffd 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
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
wrote:

> From: Jose Maria Casanova Crespo 
>
> 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 = 0xfffd 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
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Iago Toral Quiroga]

From: Jose Maria Casanova Crespo 

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 = 0xfffd 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);
return imm;
 }
 
-- 
2.14.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev