"Juan A. Suarez Romero" <jasua...@igalia.com> writes: > 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
Neither one of these test vulkan, and they do not specify a revision to use for vulkancts. If you look in the subgroups section of those builds, you will not see any vulkan suites. Vulkan is tested by the vulkacts and vulkancts_daily builds: https://mesa-ci.01.org/vulkancts/builds/9626/group/63a9f0ea7bb98050796b649e85481845 https://mesa-ci.01.org/vulkancts_daily/builds/1228/group/63a9f0ea7bb98050796b649e85481845 The vulkan suites are separated out because we didn't want vulkan tests to be triggered by piglit changes. When mesa is pushed, both the vulkancts and mesa_master builds are triggered. The naming is confusing. We should rename the builds: mesa_master* -> mesa_master_opengl* vulkancts* -> mesa_master_vulkan* > 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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
