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