On Wed, May 2, 2018 at 8:19 AM, Chema Casanova <jmcasan...@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 <jmcasan...@igalia.com > > <mailto:jmcasan...@igalia.com>> 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 <mailto:ito...@igalia.com> > > > <mailto:ito...@igalia.com <mailto:ito...@igalia.com>>> wrote: > > > > > > From: Jose Maria Casanova Crespo <jmcasan...@igalia.com > <mailto:jmcasan...@igalia.com> > > > <mailto:jmcasan...@igalia.com <mailto:jmcasan...@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 <ja...@jlekstrand.net>
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
