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

[Mark Janes]

"Juan A. Suarez Romero" <jasuarez at 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 <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
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev