<div dir="ltr"><div>Yeah, I think this is fine. We could expand the assert further to cover all possible casting cases but it's probably not worth it.</div><div> </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><div>--Jason </div></div> <div class="gmail_quote"><div dir="ltr">On Tue, Nov 20, 2018 at 4:14 AM Juan A. Suarez Romero <<a href="mailto:jasuarez@igalia.com">jasuarez@igalia.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, 2018-11-20 at 09:47 +0100, Iago Toral Quiroga wrote: > This reverts commit . > > For this to work the compiler must ensure that it never puts > the values that arrive to this helper into unsigned variables > at any point in its processing, since that would not apply sign > extension to the value and it would break the expectations here. > Unfortunately, we use uint64_t extensively to pass and copy > things around, so some times we get to this helper with values > that are not properly sign extended to 64-bit. Here is an example > for an 8-bit value that comes from a switch case: > > (gdb) p /x x > $1 = 0xffffffd6 > > The value seems to have been copied to a 32-bit value at some point > getting proper sign extension, but then copied into a uint64_t > which wont' apply sign extension, breaking the expectations of > the assertion. This is fixing dEQP-VK.spirv_assembly.type.scalar.i16.switch_vert, which failing this assertion. Specifically, it is failing when dealing with this SPIR-V instruction: OpSwitch %input_val %default -3221 %case0 3210 %case1 19597 %case2 And more specifically, when dealing with the -3221 value. Just to point out the values are int16. Additional, just to mention this wasn't caught by the Intel CI because the Vulkan CTS reference it uses to test master points to Vulkan CTS 1.1.0, which does not contain this test. What is the policty to update the testsuite references for master testing? Shouldn't use a more recent version of the test suite? The latest version is 1.1.2.2, which makes the version used as reference a bit old. Reviewed-by: Juan A. Suarez <<a href="mailto:jasuarez@igalia.com" target="_blank">jasuarez@igalia.com</a>> > --- > > FWIW, I tracked down the case where we were producing this and fixed > it, then the test that triggered this problem started to pass... but > others started to fail so I think it is best that we remove the assert > for now. If we want to keep this then I think we might need to do a > careful review of the compiler code and make sure we don't use any > unsigned types to copy things around to ensure that sign extension > happens consistently throughout. > > src/compiler/nir/nir_builder.h | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/src/compiler/nir/nir_builder.h b/src/compiler/nir/nir_builder.h > index e37aba23dc..30fa1d7ec8 100644 > --- a/src/compiler/nir/nir_builder.h > +++ b/src/compiler/nir/nir_builder.h > @@ -330,10 +330,6 @@ nir_imm_intN_t(nir_builder *build, uint64_t x, unsigned bit_size) > { > nir_const_value v; > > - assert(bit_size == 64 || > - (int64_t)x >> bit_size == 0 || > - (int64_t)x >> bit_size == -1); > - > memset(&v, 0, sizeof(v)); > assert(bit_size <= 64); > v.i64[0] = x & (~0ull >> (64 - bit_size)); _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>