Re: [Piglit] [PATCH] program_interface_query: don't expect a valid location for TCS output

2018-11-27 Thread andrey simiklit 
Hello,

Thanks a lot for review.

Regards,
Andrii.

On Tue, Nov 27, 2018 at 11:23 AM Tapani Pälli 
wrote:

> Hi;
>
> On 10/31/18 5:05 PM, asimiklit.w...@gmail.com wrote:
> > From: Andrii Simiklit 
> >
> > I guess we should not expect a valid location for:
> > "patch out vec4 tcs_patch;"
> > because this output variable is declareted
> > without "layout (location=X)" and according to spec:
> > "Not all active variables are assigned valid locations; the
> >  following variables will have an effective location of -1:
> >
> >* uniforms declared as atomic counters;
> >
> >* members of a uniform block;
> >
> >* built-in inputs, outputs, and uniforms (starting with "gl_");
> and
> >
> >* inputs or outputs not declared with a "location" layout
> qualifier,
> >  except for vertex shader inputs and fragment shader outputs."
>
>
> I've verified that this is the way it is. It took me a while to digest
> this since the test uses PROGRAM_SEPARABLE here and the rules of the
> extension are quite complicated.
>
> However, I'm convinced now and this LGTM :)
>
> Reviewed-by: Tapani Pälli 
>
>
> > Also I fixed some conflicting comments.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108612
> > Signed-off-by: Andrii Simiklit 
> > ---
> >   .../getprogramresourceiv.c   | 16 
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git
> a/tests/spec/arb_program_interface_query/getprogramresourceiv.c
> b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
> > index 2727a6c87..70212350e 100755
> > --- a/tests/spec/arb_program_interface_query/getprogramresourceiv.c
> > +++ b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
> > @@ -360,7 +360,7 @@ static const struct subtest_t subtests[] = {
> >   { GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
> > - { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
> > + { GL_LOCATION, 1, { -1 } }, /* invalid index */
> >   { 0, 0, { 0 } }}
> >},
> >{ _loc, GL_PROGRAM_INPUT, "input0", NULL, {
> > @@ -396,12 +396,12 @@ static const struct subtest_t subtests[] = {
> >   { GL_NAME_LENGTH, 1, { 6 } },
> >   { GL_TYPE, 1, { GL_FLOAT_VEC4 } },
> >   { GL_ARRAY_SIZE, 1, { 1 } },
> > - { GL_OFFSET, 1, { -1 } }, /* valid index == anything but -1 */
> > + { GL_OFFSET, 1, { -1 } }, /* invalid index */
> >   { GL_BLOCK_INDEX, 1, { -1 } }, /* invalid index */
> > - { GL_ARRAY_STRIDE, 1, { -1 } }, /* valid index == anything but -1
> */
> > + { GL_ARRAY_STRIDE, 1, { -1 } }, /* invalid index */
> >   { GL_MATRIX_STRIDE, 1, { -1 } },
> >   { GL_IS_ROW_MAJOR, 1, { 0 } },
> > - { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index ==
> anything but -1 */
> > + { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* invalid index */
> >   { GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
> > @@ -421,8 +421,8 @@ static const struct subtest_t subtests[] = {
> >  { GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
> >  { GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 0 } },
> >  { GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
> > -   { GL_LOCATION, 1, { 0 } }, /* valid index == anything but -1 */
> > -   { GL_LOCATION_INDEX, 1, { -1 } }, /* valid index == anything but
> -1 */
> > +   { GL_LOCATION, 1, { -1 } }, /* invalid index */
> > +   { GL_LOCATION_INDEX, 1, { -1 } }, /* invalid index */
> >  { GL_IS_PER_PATCH, 1, { 1 } },
> >  { 0, 0, { 0 } }}
> >   },
> > @@ -435,14 +435,14 @@ static const struct subtest_t subtests[] = {
> >   { GL_ARRAY_STRIDE, 1, { 0 } }, /* valid index == anything but -1 */
> >   { GL_MATRIX_STRIDE, 1, { 0 } },
> >   { GL_IS_ROW_MAJOR, 1, { 0 } },
> > - { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index ==
> anything but -1 */
> > + { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* invalid index */
> >   { GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
> >   { GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 1 } },
> >   { GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
> > - { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
> > + { GL_LOCATION, 1, { -1 } }, /* invalid index */
> >   { 0, 0, { 0 } }}
> >},
> >{ _std, GL_UNIFORM_BLOCK, "fs_uniform_block",
> fs_std_fs_uniform_blk, {
> >
>
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] program_interface_query: don't expect a valid location for TCS output

2018-11-27 Thread Tapani Pälli 

Hi;

On 10/31/18 5:05 PM, asimiklit.w...@gmail.com wrote:

From: Andrii Simiklit 

I guess we should not expect a valid location for:
"patch out vec4 tcs_patch;"
because this output variable is declareted
without "layout (location=X)" and according to spec:
"Not all active variables are assigned valid locations; the
 following variables will have an effective location of -1:

   * uniforms declared as atomic counters;

   * members of a uniform block;

   * built-in inputs, outputs, and uniforms (starting with "gl_"); and

   * inputs or outputs not declared with a "location" layout qualifier,
 except for vertex shader inputs and fragment shader outputs."



I've verified that this is the way it is. It took me a while to digest 
this since the test uses PROGRAM_SEPARABLE here and the rules of the 
extension are quite complicated.


However, I'm convinced now and this LGTM :)

Reviewed-by: Tapani Pälli 



Also I fixed some conflicting comments.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108612
Signed-off-by: Andrii Simiklit 
---
  .../getprogramresourceiv.c   | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/spec/arb_program_interface_query/getprogramresourceiv.c 
b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
index 2727a6c87..70212350e 100755
--- a/tests/spec/arb_program_interface_query/getprogramresourceiv.c
+++ b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
@@ -360,7 +360,7 @@ static const struct subtest_t subtests[] = {
{ GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
-   { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_LOCATION, 1, { -1 } }, /* invalid index */
{ 0, 0, { 0 } }}
   },
   { _loc, GL_PROGRAM_INPUT, "input0", NULL, {
@@ -396,12 +396,12 @@ static const struct subtest_t subtests[] = {
{ GL_NAME_LENGTH, 1, { 6 } },
{ GL_TYPE, 1, { GL_FLOAT_VEC4 } },
{ GL_ARRAY_SIZE, 1, { 1 } },
-   { GL_OFFSET, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_OFFSET, 1, { -1 } }, /* invalid index */
{ GL_BLOCK_INDEX, 1, { -1 } }, /* invalid index */
-   { GL_ARRAY_STRIDE, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_ARRAY_STRIDE, 1, { -1 } }, /* invalid index */
{ GL_MATRIX_STRIDE, 1, { -1 } },
{ GL_IS_ROW_MAJOR, 1, { 0 } },
-   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index == 
anything but -1 */
+   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* invalid index */
{ GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
@@ -421,8 +421,8 @@ static const struct subtest_t subtests[] = {
 { GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
 { GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 0 } },
 { GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
-   { GL_LOCATION, 1, { 0 } }, /* valid index == anything but -1 */
-   { GL_LOCATION_INDEX, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_LOCATION, 1, { -1 } }, /* invalid index */
+   { GL_LOCATION_INDEX, 1, { -1 } }, /* invalid index */
 { GL_IS_PER_PATCH, 1, { 1 } },
 { 0, 0, { 0 } }}
  },
@@ -435,14 +435,14 @@ static const struct subtest_t subtests[] = {
{ GL_ARRAY_STRIDE, 1, { 0 } }, /* valid index == anything but -1 */
{ GL_MATRIX_STRIDE, 1, { 0 } },
{ GL_IS_ROW_MAJOR, 1, { 0 } },
-   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index == 
anything but -1 */
+   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* invalid index */
{ GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 1 } },
{ GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
-   { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_LOCATION, 1, { -1 } }, /* invalid index */
{ 0, 0, { 0 } }}
   },
   { _std, GL_UNIFORM_BLOCK, "fs_uniform_block", fs_std_fs_uniform_blk, {


___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH] program_interface_query: don't expect a valid location for TCS output

2018-10-31 Thread asimiklit . work 
From: Andrii Simiklit 

I guess we should not expect a valid location for:
"patch out vec4 tcs_patch;"
because this output variable is declareted
without "layout (location=X)" and according to spec:
   "Not all active variables are assigned valid locations; the
following variables will have an effective location of -1:

  * uniforms declared as atomic counters;

  * members of a uniform block;

  * built-in inputs, outputs, and uniforms (starting with "gl_"); and

  * inputs or outputs not declared with a "location" layout qualifier,
except for vertex shader inputs and fragment shader outputs."

Also I fixed some conflicting comments.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108612
Signed-off-by: Andrii Simiklit 
---
 .../getprogramresourceiv.c   | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/spec/arb_program_interface_query/getprogramresourceiv.c 
b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
index 2727a6c87..70212350e 100755
--- a/tests/spec/arb_program_interface_query/getprogramresourceiv.c
+++ b/tests/spec/arb_program_interface_query/getprogramresourceiv.c
@@ -360,7 +360,7 @@ static const struct subtest_t subtests[] = {
{ GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
-   { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_LOCATION, 1, { -1 } }, /* invalid index */
{ 0, 0, { 0 } }}
  },
  { _loc, GL_PROGRAM_INPUT, "input0", NULL, {
@@ -396,12 +396,12 @@ static const struct subtest_t subtests[] = {
{ GL_NAME_LENGTH, 1, { 6 } },
{ GL_TYPE, 1, { GL_FLOAT_VEC4 } },
{ GL_ARRAY_SIZE, 1, { 1 } },
-   { GL_OFFSET, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_OFFSET, 1, { -1 } }, /* invalid index */
{ GL_BLOCK_INDEX, 1, { -1 } }, /* invalid index */
-   { GL_ARRAY_STRIDE, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_ARRAY_STRIDE, 1, { -1 } }, /* invalid index */
{ GL_MATRIX_STRIDE, 1, { -1 } },
{ GL_IS_ROW_MAJOR, 1, { 0 } },
-   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index == 
anything but -1 */
+   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* invalid index */
{ GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
@@ -421,8 +421,8 @@ static const struct subtest_t subtests[] = {
{ GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
-   { GL_LOCATION, 1, { 0 } }, /* valid index == anything but -1 */
-   { GL_LOCATION_INDEX, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_LOCATION, 1, { -1 } }, /* invalid index */
+   { GL_LOCATION_INDEX, 1, { -1 } }, /* invalid index */
{ GL_IS_PER_PATCH, 1, { 1 } },
{ 0, 0, { 0 } }}
 },
@@ -435,14 +435,14 @@ static const struct subtest_t subtests[] = {
{ GL_ARRAY_STRIDE, 1, { 0 } }, /* valid index == anything but -1 */
{ GL_MATRIX_STRIDE, 1, { 0 } },
{ GL_IS_ROW_MAJOR, 1, { 0 } },
-   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* valid index == 
anything but -1 */
+   { GL_ATOMIC_COUNTER_BUFFER_INDEX, 1, { -1 } }, /* invalid index */
{ GL_REFERENCED_BY_VERTEX_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_CONTROL_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_TESS_EVALUATION_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_GEOMETRY_SHADER, 1, { 0 } },
{ GL_REFERENCED_BY_FRAGMENT_SHADER, 1, { 1 } },
{ GL_REFERENCED_BY_COMPUTE_SHADER, 1, { 0 } },
-   { GL_LOCATION, 1, { -1 } }, /* valid index == anything but -1 */
+   { GL_LOCATION, 1, { -1 } }, /* invalid index */
{ 0, 0, { 0 } }}
  },
  { _std, GL_UNIFORM_BLOCK, "fs_uniform_block", fs_std_fs_uniform_blk, {
-- 
2.17.1

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit