[Mesa-dev] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"

[Juan A. Suarez Romero]

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 <jasuarez at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev