On Tue, 2018-11-20 at 11:13 +0100, Juan A. Suarez Romero wrote: > 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. >
I'm realizing this is not entirely true. There are right now two kind of test branches for master: mesa_master and mesa_master_daily. The former uses vulkan- cts-1.0.2 as reference (plus some changes), and the later uses vulkan-cts- 1.1.2.1 as reference (plus some changes). https://mesa-ci.01.org/mesa_master/builds/14578/group/63a9f0ea7bb98050796b649e85481845 https://mesa-ci.01.org/mesa_master_daily/builds/4596/group/63a9f0ea7bb98050796b649e85481845 So in theory the later should catch the error. But apparently, they aren't running the vulkancts testsuite, or at least, I don't see it in the list of testsuites run. > 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 <jasua...@igalia.com> > > > --- > > > > 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 > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
