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

[Jason Ekstrand]

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.

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

--Jason

On Tue, Nov 20, 2018 at 4:14 AM Juan A. Suarez Romero <jasuarez at igalia.com>
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.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181120/f9dd53c5/attachment.html>