Re: [Mesa-dev] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"
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
On Tue, Nov 20, 2018 at 4:14 AM 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 = 0xffd6
> >
> > 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
>
> > ---
> >
> > 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(, 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
Re: [Mesa-dev] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"
"Juan A. Suarez Romero" 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 = 0xffd6
>> >
>> > 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
>>
>> > ---
>> >
>> > 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(, 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
Re: [Mesa-dev] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"
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 = 0xffd6
> >
> > 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
>
> > ---
> >
> > 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(, 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
Re: [Mesa-dev] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"
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 = 0xffd6
>
> 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
> ---
>
> 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(, 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] [PATCH] Revert "nir/builder: Assert that intN_t immediates fit"
This reverts commit 1f29f4db1e867357a119c0c7c34fb54dc27fb682.
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 = 0xffd6
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.
---
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(, 0, sizeof(v));
assert(bit_size <= 64);
v.i64[0] = x & (~0ull >> (64 - bit_size));
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
