Bug 109532 - ir_variable has maximum access out of bounds -- but it's not out of bounds
Summary: ir_variable has maximum access out of bounds -- but it's not out of bounds
Status: NEW
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
: 109260 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-02-02 09:10 UTC by Ilia Mirkin
Modified: 2019-02-21 15:38 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
gdb session + generated shaders (19.32 KB, text/plain)
2019-02-02 09:10 UTC, Ilia Mirkin
Details
this simple program helps me to reproduce this issue. (5.99 KB, application/zip)
2019-02-04 16:38 UTC, asimiklit
Details
one more issue. (5.36 KB, text/plain)
2019-02-11 11:54 UTC, asimiklit
Details
patch to disallow an elimination of the first unused ub/ssbo array elements (5.53 KB, patch)
2019-02-21 15:38 UTC, asimiklit
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ilia Mirkin 2019-02-02 09:10:37 UTC
Created attachment 143274 [details]
gdb session + generated shaders

This dEQP test asserts, at least on nouveau, but I don't see what would cause it to be driver-specific.

dEQP-GLES31.functional.ssbo.layout.random.all_per_block_buffers.18

This appears to happen because it doesn't understand the interface array index properly? This message is printed before the abort:

ir_variable has maximum access out of bounds (1 vs 0)

For some reason it thinks that the array length is 1 when it should be 2. I recall interfaces being treated rather differently than most things, so perhaps related.
Comment 1 Timothy Arceri 2019-02-02 20:33:08 UTC
I believe we split (maybe eliminate unused blocks?). Looks like the shader only access blockB[1] and not blockB[0] so I assume this has something to do with it. Probably not updating something correctly when doing so.
Comment 2 Ilia Mirkin 2019-02-03 00:58:03 UTC
Looks like this is happening in link_uniform_blocks. The type gets updated:

      if (b->array != NULL &&
          (b->type->without_array()->interface_packing ==
           GLSL_INTERFACE_PACKING_PACKED)) {
         b->type = resize_block_array(b->type, b->array);
         b->var->type = b->type;
      }

But no change to the corresponding max_data_access. However doing the naive thing here didn't help -- the shader fails. I didn't investigate why. I think create_buffer_blocks also needs to be involved somehow.
Comment 3 Mark Janes 2019-02-03 03:37:56 UTC
This test was broken in the dEQP suite recently for m32 i965 platforms by:

237916d03377a469e30dd36087738b069f83a19a
Author:     xinglong.zhu <xinglong.zhu@spreadtrum.com>
Commit:     Patrick Shi <patrick.shi@unisoc.com>
CommitDate: Sat Jan 5 12:46:03 2019 +0000

gles31:es31fSSBOLayoutTests limit m_maxBlocks and m_maxBlockMembers for low ram device

Memory requirement for cts CtsDeqpTestCases
dEQP-GLES31.functional.ssbo.layout.random.all_shared_buffer#5
reduced from 230MB to 36MB

Issue: 121364689
Test: run cts -m CtsDeqpTestCases -t dEQP-GLES31.functional.ssbo.layout.random.all_shared_buffer#5

Change-Id: I2a66263f041265e8697e4b270132b9889486ce4f


I contacted the author and committer, but they had no way to verify on mesa.

I'll forward to deqp-external-requests@google.com
Comment 4 Ilia Mirkin 2019-02-03 14:26:25 UTC
(In reply to Mark Janes from comment #3)
> This test was broken in the dEQP suite recently for m32 i965 platforms by:

FWIW I don't think this is a test bug. The shaders used appear perfectly reasonable. It seems like the commit in question would have changed the generated shader a bit, which could be triggering the different behavior (seems likely, in fact). But ultimately this is a mesa issue, not a dEQP issue.
Comment 5 asimiklit 2019-02-04 15:30:15 UTC
Hi,

I managed to reproduce this issue using provided shader under KBL with latest mesa from git.
This happens with 'BlockB' field:
ir->data.max_array_access is 1
and
ir->type->length is 1 too

I am going to continue my investigation.

Thanks,
Andrii.
Comment 6 asimiklit 2019-02-04 16:38:39 UTC
Created attachment 143288 [details]
this simple program helps me to reproduce this issue.

just share my simple reproducer)

Run it in this way:

   simple_reproducer shader.comp
Comment 7 asimiklit 2019-02-05 15:19:14 UTC
(In reply to Ilia Mirkin from comment #2)
> Looks like this is happening in link_uniform_blocks. The type gets updated:
> 
>       if (b->array != NULL &&
>           (b->type->without_array()->interface_packing ==
>            GLSL_INTERFACE_PACKING_PACKED)) {
>          b->type = resize_block_array(b->type, b->array);
>          b->var->type = b->type;
>       }
> 
> But no change to the corresponding max_data_access. However doing the naive
> thing here didn't help -- the shader fails. I didn't investigate why. I
> think create_buffer_blocks also needs to be involved somehow.

Thanks a lot your message was very helpful to find the start point of investigation.

Looks like this thing is helped:

---
 src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/compiler/glsl/link_uniform_blocks.cpp b/src/compiler/glsl/link_uniform_blocks.cpp
index 0b890586298..5197870a6d9 100644
--- a/src/compiler/glsl/link_uniform_blocks.cpp
+++ b/src/compiler/glsl/link_uniform_blocks.cpp
@@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
       if (b->array != NULL &&
           (b->type->without_array()->interface_packing ==
            GLSL_INTERFACE_PACKING_PACKED)) {
+         const int new_max_array_access = (int)(b->array->num_array_elements - 1);
          b->type = resize_block_array(b->type, b->array);
          b->var->type = b->type;
+
+         assert(((int)b->array->array_elements[new_max_array_access] ==
+                b->var->data.max_array_access ||
+                b->var->data.max_array_access == -1) && "Is last index is proper");
+
+         b->var->data.max_array_access = new_max_array_access;
       }
 
       block_size.num_active_uniforms = 0;
-- 
2.17.1

Did you mean the same thing saying "naive thing" or this change helps?
Comment 8 Ilia Mirkin 2019-02-05 15:27:02 UTC
(In reply to asimiklit from comment #7)
> (In reply to Ilia Mirkin from comment #2)
> > Looks like this is happening in link_uniform_blocks. The type gets updated:
> > 
> >       if (b->array != NULL &&
> >           (b->type->without_array()->interface_packing ==
> >            GLSL_INTERFACE_PACKING_PACKED)) {
> >          b->type = resize_block_array(b->type, b->array);
> >          b->var->type = b->type;
> >       }
> > 
> > But no change to the corresponding max_data_access. However doing the naive
> > thing here didn't help -- the shader fails. I didn't investigate why. I
> > think create_buffer_blocks also needs to be involved somehow.
> 
> Thanks a lot your message was very helpful to find the start point of
> investigation.
> 
> Looks like this thing is helped:
> 
> ---
>  src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/compiler/glsl/link_uniform_blocks.cpp
> b/src/compiler/glsl/link_uniform_blocks.cpp
> index 0b890586298..5197870a6d9 100644
> --- a/src/compiler/glsl/link_uniform_blocks.cpp
> +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> @@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
>        if (b->array != NULL &&
>            (b->type->without_array()->interface_packing ==
>             GLSL_INTERFACE_PACKING_PACKED)) {
> +         const int new_max_array_access =
> (int)(b->array->num_array_elements - 1);
>           b->type = resize_block_array(b->type, b->array);
>           b->var->type = b->type;
> +
> +         assert(((int)b->array->array_elements[new_max_array_access] ==
> +                b->var->data.max_array_access ||
> +                b->var->data.max_array_access == -1) && "Is last index is
> proper");
> +
> +         b->var->data.max_array_access = new_max_array_access;
>        }
>  
>        block_size.num_active_uniforms = 0;
> -- 
> 2.17.1
> 
> Did you mean the same thing saying "naive thing" or this change helps?

Well, as I recall the thing I tried was just saying max_array_access = 0 or -1 unconditionally. It could also have been due to some secondary issue with nouveau that this failed -- like I said, I didn't investigate. I'll have a closer look tonight.

You're basically setting

b->var->data.max_array_access = b->type->length - 1

right?
Comment 9 asimiklit 2019-02-05 16:02:40 UTC
(In reply to Ilia Mirkin from comment #8)
> (In reply to asimiklit from comment #7)
> > (In reply to Ilia Mirkin from comment #2)
> > > Looks like this is happening in link_uniform_blocks. The type gets updated:
> > > 
> > >       if (b->array != NULL &&
> > >           (b->type->without_array()->interface_packing ==
> > >            GLSL_INTERFACE_PACKING_PACKED)) {
> > >          b->type = resize_block_array(b->type, b->array);
> > >          b->var->type = b->type;
> > >       }
> > > 
> > > But no change to the corresponding max_data_access. However doing the naive
> > > thing here didn't help -- the shader fails. I didn't investigate why. I
> > > think create_buffer_blocks also needs to be involved somehow.
> > 
> > Thanks a lot your message was very helpful to find the start point of
> > investigation.
> > 
> > Looks like this thing is helped:
> > 
> > ---
> >  src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/link_uniform_blocks.cpp
> > b/src/compiler/glsl/link_uniform_blocks.cpp
> > index 0b890586298..5197870a6d9 100644
> > --- a/src/compiler/glsl/link_uniform_blocks.cpp
> > +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> > @@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
> >        if (b->array != NULL &&
> >            (b->type->without_array()->interface_packing ==
> >             GLSL_INTERFACE_PACKING_PACKED)) {
> > +         const int new_max_array_access =
> > (int)(b->array->num_array_elements - 1);
> >           b->type = resize_block_array(b->type, b->array);
> >           b->var->type = b->type;
> > +
> > +         assert(((int)b->array->array_elements[new_max_array_access] ==
> > +                b->var->data.max_array_access ||
> > +                b->var->data.max_array_access == -1) && "Is last index is
> > proper");
> > +
> > +         b->var->data.max_array_access = new_max_array_access;
> >        }
> >  
> >        block_size.num_active_uniforms = 0;
> > -- 
> > 2.17.1
> > 
> > Did you mean the same thing saying "naive thing" or this change helps?
> 
> Well, as I recall the thing I tried was just saying max_array_access = 0 or
> -1 unconditionally. It could also have been due to some secondary issue with
> nouveau that this failed -- like I said, I didn't investigate. I'll have a
> closer look tonight.

Got it.

> 
> You're basically setting
> 
> b->var->data.max_array_access = b->type->length - 1
> 
> right?

Yes. I guess that the optimized array size - 1 is a new max_array_access. 


But in case if optimization algorithm has some unexpected behavior and could left some unused elements in array I guess the search will be required:
for (int i = 0; i < b->array->num_array_elements; i++)
{
   if(b->var->data.max_array_access == b->array->array_elements[i])
   {
     b->var->data.max_array_access = i;
     break;
   }
}(In reply to Ilia Mirkin from comment #8)
> (In reply to asimiklit from comment #7)
> > (In reply to Ilia Mirkin from comment #2)
> > > Looks like this is happening in link_uniform_blocks. The type gets updated:
> > > 
> > >       if (b->array != NULL &&
> > >           (b->type->without_array()->interface_packing ==
> > >            GLSL_INTERFACE_PACKING_PACKED)) {
> > >          b->type = resize_block_array(b->type, b->array);
> > >          b->var->type = b->type;
> > >       }
> > > 
> > > But no change to the corresponding max_data_access. However doing the naive
> > > thing here didn't help -- the shader fails. I didn't investigate why. I
> > > think create_buffer_blocks also needs to be involved somehow.
> > 
> > Thanks a lot your message was very helpful to find the start point of
> > investigation.
> > 
> > Looks like this thing is helped:
> > 
> > ---
> >  src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/link_uniform_blocks.cpp
> > b/src/compiler/glsl/link_uniform_blocks.cpp
> > index 0b890586298..5197870a6d9 100644
> > --- a/src/compiler/glsl/link_uniform_blocks.cpp
> > +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> > @@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
> >        if (b->array != NULL &&
> >            (b->type->without_array()->interface_packing ==
> >             GLSL_INTERFACE_PACKING_PACKED)) {
> > +         const int new_max_array_access =
> > (int)(b->array->num_array_elements - 1);
> >           b->type = resize_block_array(b->type, b->array);
> >           b->var->type = b->type;
> > +
> > +         assert(((int)b->array->array_elements[new_max_array_access] ==
> > +                b->var->data.max_array_access ||
> > +                b->var->data.max_array_access == -1) && "Is last index is
> > proper");
> > +
> > +         b->var->data.max_array_access = new_max_array_access;
> >        }
> >  
> >        block_size.num_active_uniforms = 0;
> > -- 
> > 2.17.1
> > 
> > Did you mean the same thing saying "naive thing" or this change helps?
> 
> Well, as I recall the thing I tried was just saying max_array_access = 0 or
> -1 unconditionally. It could also have been due to some secondary issue with
> nouveau that this failed -- like I said, I didn't investigate. I'll have a
> closer look tonight.

Got it.

> 
> You're basically setting
> 
> b->var->data.max_array_access = b->type->length - 1
> 
> right?

Yes. I guess that the optimized array size - 1 is a new max_array_access.

But in case if optimization algorithm has some "unexpected behavior" and could left some unused elements at the end of array for some reason I guess the some search may be required:

for (int i = 0; i < b->array->num_array_elements; i++)
{
   if(b->var->data.max_array_access == b->array->array_elements[i])
   {
     b->var->data.max_array_access = i;
     break;
   }
}
Comment 10 Ilia Mirkin 2019-02-05 16:04:38 UTC
Does your patch make the test pass on i965? If so, I'll have to figure out where in st/mesa or nouveau it dies.
Comment 11 asimiklit 2019-02-05 16:26:52 UTC
The i965 tests is in progress and result will be appear here (in an hour I think):

https://mesa-ci.01.org/global_logic/builds/

It will be "Build # 70"

The commit which is under test:
https://gitlab.freedesktop.org/GL/mesa/commit/526a9fee0e40a450bc71092b27a3b89cbbf93fd7
Comment 12 Mark Janes 2019-02-05 17:29:51 UTC
(In reply to asimiklit from comment #11)
> https://mesa-ci.01.org/global_logic/builds/
> 
> It will be "Build # 70"
> 

With this build, i965 reports test failure instead of assertion (the same status as for m32).  Before the dEQP change, this test passed.

Since the status has been different for 32-bit binaries, I'm rebuilding with that configuration (see build # 71)
Comment 13 andrii simiklit 2019-02-05 19:00:04 UTC
(In reply to Mark Janes from comment #12)
> (In reply to asimiklit from comment #11)
> > https://mesa-ci.01.org/global_logic/builds/
> > 
> > It will be "Build # 70"
> > 
> 
> With this build, i965 reports test failure instead of assertion (the same
> status as for m32).  Before the dEQP change, this test passed.
> 
> Since the status has been different for 32-bit binaries, I'm rebuilding with
> that configuration (see build # 71)

Thank you) But unfortunately the tests failed so guess I didn't consider something. I am going to continue my investigation)
Comment 14 Ilia Mirkin 2019-02-05 19:21:59 UTC
(In reply to andrii simiklit from comment #13)
> Thank you) But unfortunately the tests failed so guess I didn't consider
> something. I am going to continue my investigation)

Did the test actually pass for you? Or just not crash? When I made my "naive" change, it fixed the crash, but the test continued to fail.
Comment 15 Mark Janes 2019-02-05 20:15:15 UTC
(In reply to Ilia Mirkin from comment #14)
> Did the test actually pass for you? Or just not crash? When I made my
> "naive" change, it fixed the crash, but the test continued to fail.

On i965, tests did not crash, but they continued to fail.
Comment 16 andrii simiklit 2019-02-06 07:51:15 UTC
(In reply to Ilia Mirkin from comment #14)
> (In reply to andrii simiklit from comment #13)
> > Thank you) But unfortunately the tests failed so guess I didn't consider
> > something. I am going to continue my investigation)
> 
> Did the test actually pass for you? Or just not crash? When I made my
> "naive" change, it fixed the crash, but the test continued to fail.

Sorry for misudestanding (In reply to Ilia Mirkin from comment #14)
> (In reply to andrii simiklit from comment #13)
> > Thank you) But unfortunately the tests failed so guess I didn't consider
> > something. I am going to continue my investigation)
> 
> Did the test actually pass for you? Or just not crash? When I made my
> "naive" change, it fixed the crash, but the test continued to fail.

Oops I thought you ask just about crash. Sorry for confusion.
So I have the same results. I will try to understand why test fails today.
Comment 17 Ian Romanick 2019-02-08 20:55:22 UTC
(In reply to asimiklit from comment #6)
> Created attachment 143288 [details]
> this simple program helps me to reproduce this issue.
> 
> just share my simple reproducer)
> 
> Run it in this way:
> 
>    simple_reproducer shader.comp

It seems like this could be made into a piglit test that could be compiled with glslparsertest.  If you haven't already submitted such a test, please do. :)
Comment 18 Ian Romanick 2019-02-08 21:48:49 UTC
(In reply to asimiklit from comment #7)
> (In reply to Ilia Mirkin from comment #2)
> > Looks like this is happening in link_uniform_blocks. The type gets updated:
> > 
> >       if (b->array != NULL &&
> >           (b->type->without_array()->interface_packing ==
> >            GLSL_INTERFACE_PACKING_PACKED)) {
> >          b->type = resize_block_array(b->type, b->array);
> >          b->var->type = b->type;
> >       }
> > 
> > But no change to the corresponding max_data_access. However doing the naive
> > thing here didn't help -- the shader fails. I didn't investigate why. I
> > think create_buffer_blocks also needs to be involved somehow.
> 
> Thanks a lot your message was very helpful to find the start point of
> investigation.
> 
> Looks like this thing is helped:
> 
> ---
>  src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/compiler/glsl/link_uniform_blocks.cpp
> b/src/compiler/glsl/link_uniform_blocks.cpp
> index 0b890586298..5197870a6d9 100644
> --- a/src/compiler/glsl/link_uniform_blocks.cpp
> +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> @@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
>        if (b->array != NULL &&
>            (b->type->without_array()->interface_packing ==
>             GLSL_INTERFACE_PACKING_PACKED)) {
> +         const int new_max_array_access =
> (int)(b->array->num_array_elements - 1);
>           b->type = resize_block_array(b->type, b->array);
>           b->var->type = b->type;
> +
> +         assert(((int)b->array->array_elements[new_max_array_access] ==
> +                b->var->data.max_array_access ||
> +                b->var->data.max_array_access == -1) && "Is last index is
> proper");
> +
> +         b->var->data.max_array_access = new_max_array_access;
>        }
>  
>        block_size.num_active_uniforms = 0;

I don't think this has any chance of being correct.  max_array_access is set based on actual, known accesses to the array.  Changing that arbitrarily is just lying to the rest of the compiler.  Nothing good can come from that.  Since the type of the array is now wrong, my guess is that the test fails (instead of crashes) because the part of the array that has the data the shader wants is optimized away.

How does the mismatch between max_array_access and the type occur in the first place?  My guess is that this is the source of the bug.  They *must* agree at some point, or an earlier part of the compile would have generated an error (as required by the spec).
Comment 19 andrii simiklit 2019-02-09 20:33:28 UTC
(In reply to Ian Romanick from comment #18)
> (In reply to asimiklit from comment #7)
> > (In reply to Ilia Mirkin from comment #2)
> > > Looks like this is happening in link_uniform_blocks. The type gets updated:
> > > 
> > >       if (b->array != NULL &&
> > >           (b->type->without_array()->interface_packing ==
> > >            GLSL_INTERFACE_PACKING_PACKED)) {
> > >          b->type = resize_block_array(b->type, b->array);
> > >          b->var->type = b->type;
> > >       }
> > > 
> > > But no change to the corresponding max_data_access. However doing the naive
> > > thing here didn't help -- the shader fails. I didn't investigate why. I
> > > think create_buffer_blocks also needs to be involved somehow.
> > 
> > Thanks a lot your message was very helpful to find the start point of
> > investigation.
> > 
> > Looks like this thing is helped:
> > 
> > ---
> >  src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/link_uniform_blocks.cpp
> > b/src/compiler/glsl/link_uniform_blocks.cpp
> > index 0b890586298..5197870a6d9 100644
> > --- a/src/compiler/glsl/link_uniform_blocks.cpp
> > +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> > @@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
> >        if (b->array != NULL &&
> >            (b->type->without_array()->interface_packing ==
> >             GLSL_INTERFACE_PACKING_PACKED)) {
> > +         const int new_max_array_access =
> > (int)(b->array->num_array_elements - 1);
> >           b->type = resize_block_array(b->type, b->array);
> >           b->var->type = b->type;
> > +
> > +         assert(((int)b->array->array_elements[new_max_array_access] ==
> > +                b->var->data.max_array_access ||
> > +                b->var->data.max_array_access == -1) && "Is last index is
> > proper");
> > +
> > +         b->var->data.max_array_access = new_max_array_access;
> >        }
> >  
> >        block_size.num_active_uniforms = 0;
> 
> I don't think this has any chance of being correct.  max_array_access is set
> based on actual, known accesses to the array.  Changing that arbitrarily is
> just lying to the rest of the compiler.  Nothing good can come from that. 
> Since the type of the array is now wrong, my guess is that the test fails
> (instead of crashes) because the part of the array that has the data the
> shader wants is optimized away.
This is changed not arbitrarily. All elements of the new optimized array type
are 100% used so index of the last element of this array is a max_array_access, no?

> 
> How does the mismatch between max_array_access and the type occur in the
> first place?  My guess is that this is the source of the bug.  They *must*
> agree at some point, or an earlier part of the compile would have generated
> an error (as required by the spec).
Do you know where the location of the code which should leads them to agreement?
Unfortunately I didn't find this code. 
Shouldn't it be correct to lead them to agreement here?

https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/compiler/glsl/link_uniform_blocks.cpp#L441
Seems like a new optimized array type should contain used/active instances only
so it should be correct to suppose that last element of this array is 100% used
and max_array_access can points to this element. Didn't I consider something here?



About deqp test, it fails in such cases:

layout(local_size_x = 1) in;

layout(packed, binding = 0) buffer BlockB
{
        float b[1];
} blockB[2];

void main (void)
{
    blockB[1].b[0] = 77777.77777;
}

and looks like problem is here:
https://android.googlesource.com/platform/external/deqp/+/master/modules/gles31/functional/es31fSSBOLayoutCase.cpp#2249

deqp test uses binding point 1 for BlockB[1] but when we use 'packed' layout the BlockB[0] instance
is removed due to unuse and there is just one array instance BlockB[1] with assigned binding point 0 instead of 1.

Looks like it should be something like this:
   if (layoutNdx >= 0)
   {
      const BlockLocation& blockLoc = blockLocations[layoutNdx];
      if (blockLoc.size > 0)
         gl.bindBufferRange(GL_SHADER_STORAGE_BUFFER, bindingPoint, buffers[blockLoc.index].buffer, blockLoc.offset, blockLoc.size);
   
      bindingPoint += 1;
   }
   else if((block.getFlags() & LAYOUT_PACKED) == 0)
   {
      bindingPoint += 1;
   }

I read the spec and looks like it is acceptable to remove the instance of array if it isn't active.
I will provide the patch (to test it on Intel CI if possible) and more details on Monday
(because I left everything on work machine).

Thanks,
Andrii.
Comment 20 asimiklit 2019-02-11 11:54:02 UTC
Created attachment 143359 [details]
one more issue.

ERROR: ac_numPassed = 0, expected 1

void main (void)
{
	bool allOk = true;
	allOk = allOk &amp;&amp; compare_mat4(blockB[1].b[0][0], mat4(-8.0, -2.0, -1.0, 3.0, 2.0, 4.0, 1.0, -7.0, 8.0, -7.0, -1.0, 4.0, -2.0, -2.0, -6.0, 1.0));
	allOk = allOk &amp;&amp; compare_mat4(blockB[1].b[0][1], mat4(-6.0, -2.0, 2.0, 7.0, -7.0, 8.0, -2.0, -7.0, 2.0, 0.0, 3.0, 9.0, 8.0, 2.0, -6.0, -2.0));
	allOk = allOk &amp;&amp; compare_int(f, 9);
	allOk = allOk &amp;&amp; compare_ivec4(blockD.g[0], ivec4(8, 4, -3, 0));
	allOk = allOk &amp;&amp; compare_ivec4(blockD.g[1], ivec4(-9, 7, -9, 8));
	if (allOk)
		atomicCounterIncrement(ac_numPassed);

	blockB[1].b[0][0] = mat4(-6.0, -7.0, -9.0, 8.0, 3.0, -8.0, 9.0, -1.0, 5.0, 3.0, -9.0, -4.0, -8.0, -6.0, 9.0, 0.0);
	blockB[1].b[0][1] = mat4(-4.0, 9.0, -2.0, 0.0, 2.0, -3.0, 5.0, 0.0, 1.0, -2.0, -3.0, 2.0, 2.0, -6.0, 2.0, -8.0);
	c = mat4x3(-5.0, -3.0, -3.0, 0.0, 8.0, -9.0, 2.0, -4.0, 1.0, 8.0, 7.0, -4.0);
	blockD.g[0] = ivec4(-4, -3, -8, 8);
	blockD.g[1] = ivec4(-8, 3, -1, 4);
}

Hmm, unfortunately there is some additional issue in the mesa or deqp.
So some variable were copied incorrectly by test or mesa or we lost something...
So I will investigate it too.

(Shared code is in attachment)
Comment 21 asimiklit 2019-02-11 13:49:50 UTC
So the previously mentioned issue was due to issue in my fix. 
I fixed my it and locally I have the following results:

./deqp-gles31 -n dEQP-GLES31.functional.ssbo.*

Test run totals:
  Passed:        2061/2061 (100.0%)
  Failed:        0/2061 (0.0%)
  Not supported: 0/2061 (0.0%)
  Warnings:      0/2061 (0.0%)


It with mesa assertion fix and with deqp fix.
Without the mesa fix it is still crashes even if the deqp fix is applied:
   Test case 'dEQP-GLES31.functional.ssbo.layout.random.all_per_block_buffers.18'..
   ir_variable has maximum access out of bounds (1 vs 0)
   Aborted (core dumped)

deqp: 
   https://github.com/asimiklit/deqp/commit/91cff8150944213f6da533e281ee76d95ca00f21

mesa:
   https://gitlab.freedesktop.org/GL/mesa/commit/526a9fee0e40a450bc71092b27a3b89cbbf93fd7

So I am actually not 100% sure about which binding point should be assigned 
for BlockB[1] instance in the following case:

   layout(local_size_x = 1) in;

   layout(packed, binding = 0) buffer BlockB
   {
           float b[1];
   } blockB[2];

   void main (void)
   {
       blockB[1].b[0] = 77777.77777;
   }

But according to next comment it should be 0 because of the BlockB[0] was optimized
and there is the BlockB[1] only:

   /* The ARB_shading_language_420pack spec says:
    *
    *    If the binding identifier is used with a uniform block instanced as
    *    an array then the first element of the array takes the specified
    *    block binding and each subsequent element takes the next consecutive
    *    uniform block binding point.
    */

https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/compiler/glsl/link_uniform_blocks.cpp#L279-310

Mark is it possible to check both patches under Intel CI?

mesa:
   fork: https://gitlab.freedesktop.org/asimiklit/mesa/
   branch: max_array_access

deqp:
   fork: https://github.com/asimiklit/deqp/
   branch: array_instance_binding_fix
Comment 22 Mark Janes 2019-02-11 16:07:28 UTC
The combination of deqp/mesa patches passed CI.  The only failure was 

  piglit.spec.!opengl 1_1.tex-upside-down-miptree

This test is new and was fixed on mesa master.  The fix is probably not in the tested developer branch.

You should contact deqp-external-requests@google.com to make your case that the deqp test is buggy.
Comment 23 asimiklit 2019-02-11 16:21:37 UTC
(In reply to Mark Janes from comment #22)
> The combination of deqp/mesa patches passed CI.  The only failure was 
> 
>   piglit.spec.!opengl 1_1.tex-upside-down-miptree
> 
> This test is new and was fixed on mesa master.  The fix is probably not in
> the tested developer branch.
> 
> You should contact deqp-external-requests@google.com to make your case that
> the deqp test is buggy.

Cool, thanks a lot.
I will contact them tomorrow)
Comment 24 Ilia Mirkin 2019-02-11 17:18:05 UTC
"""
But according to next comment it should be 0 because of the BlockB[0] was optimized
and there is the BlockB[1] only:

   /* The ARB_shading_language_420pack spec says:
    *
    *    If the binding identifier is used with a uniform block instanced as
    *    an array then the first element of the array takes the specified
    *    block binding and each subsequent element takes the next consecutive
    *    uniform block binding point.
    */
"""

I don't think that's enough justification for the "mesa" way of doing it. Whether a block is eliminated or not is not specified by GLSL spec. Would be good to get some more experienced opinions on this. [Not mine...]
Comment 25 asimiklit 2019-02-12 12:22:19 UTC
(In reply to asimiklit from comment #23)
> (In reply to Mark Janes from comment #22)
> > The combination of deqp/mesa patches passed CI.  The only failure was 
> > 
> >   piglit.spec.!opengl 1_1.tex-upside-down-miptree
> > 
> > This test is new and was fixed on mesa master.  The fix is probably not in
> > the tested developer branch.
> > 
> > You should contact deqp-external-requests@google.com to make your case that
> > the deqp test is buggy.
> 
> Cool, thanks a lot.
> I will contact them tomorrow)

I don't know, is it make sense to contact deqp-external-requests@google.com
with questions about specs they used to write this test, according to the last Ilia comment?
I think I rather won't contact them until we have 100% explanation how it should work.
I have been trying to find some explanation how we should assign binding points for
the ssbo array instances with packed layout case but
unfortunately didn't find any concrete answer yet.

It is possible that the deqp fix will migrate to mesa depends on answer which we will find.
Comment 26 asimiklit 2019-02-12 12:59:36 UTC
(In reply to Ian Romanick from comment #17)
> (In reply to asimiklit from comment #6)
> > Created attachment 143288 [details]
> > this simple program helps me to reproduce this issue.
> > 
> > just share my simple reproducer)
> > 
> > Run it in this way:
> > 
> >    simple_reproducer shader.comp
> 
> It seems like this could be made into a piglit test that could be compiled
> with glslparsertest.  If you haven't already submitted such a test, please
> do. :)

I work on it)
Comment 27 asimiklit 2019-02-15 11:58:30 UTC
(In reply to Ilia Mirkin from comment #24)
> """
> But according to next comment it should be 0 because of the BlockB[0] was
> optimized
> and there is the BlockB[1] only:
> 
>    /* The ARB_shading_language_420pack spec says:
>     *
>     *    If the binding identifier is used with a uniform block instanced as
>     *    an array then the first element of the array takes the specified
>     *    block binding and each subsequent element takes the next consecutive
>     *    uniform block binding point.
>     */
> """
> 
> I don't think that's enough justification for the "mesa" way of doing it.
> Whether a block is eliminated or not is not specified by GLSL spec. Would be
> good to get some more experienced opinions on this. [Not mine...]

I have asked about it here:
https://github.com/KhronosGroup/OpenGL-API/issues/46
Hope that somebody will clarify it)
Comment 28 Mark Janes 2019-02-15 17:25:19 UTC
Chris Forbes at google found a patch that explains at least some of the reason for this test regression:

  https://android-review.googlesource.com/c/platform/external/deqp/+/901894

I just realized that this bug is a dupe the one I opened when the test regressed.  Since the discussion is here, I'll close that one.
Comment 29 Mark Janes 2019-02-15 17:26:46 UTC
*** Bug 109260 has been marked as a duplicate of this bug. ***
Comment 30 Mark Janes 2019-02-15 18:55:49 UTC
(In reply to Mark Janes from comment #28)
>   https://android-review.googlesource.com/c/platform/external/deqp/+/901894

Mesa still asserts with this fix.  I also tested Andrii's mesa patch with the dEQP fix and the test fails.

Since non-mesa drivers have found issues with the original dEQP change, I suspect there are still deeper problems with the test.
Comment 31 andrii simiklit 2019-02-15 20:44:30 UTC
(In reply to Mark Janes from comment #30)
> (In reply to Mark Janes from comment #28)
> >   https://android-review.googlesource.com/c/platform/external/deqp/+/901894
> 
> Mesa still asserts with this fix.  I also tested Andrii's mesa patch with
> the dEQP fix and the test fails.
Do you mean the Chris's dEQP fix here, yes?
But looks like the mentioned Chris's dEQP fix considers some GL limitations and doesn't affect the expectations of binding points.

Also the assertion is a separate issue, I created the piglit test for that:
https://patchwork.freedesktop.org/patch/286287/
But yes, we unable to fix the test fail without assertion because of crash :-)

> 
> Since non-mesa drivers have found issues with the original dEQP change, I
> suspect there are still deeper problems with the test.
Possible they have the same issue with binding points mismatch after optimizations by glsl compiler. 
They could try this fix/hack for deqp which is already helped us:
https://github.com/asimiklit/deqp/commit/91cff8150944213f6da533e281ee76d95ca00f21
If it helps them we will know that it is a common issue and it could expedite this:
https://github.com/KhronosGroup/OpenGL-API/issues/46
Comment 32 andrii simiklit 2019-02-16 10:09:31 UTC
(In reply to andrii simiklit from comment #31)
> (In reply to Mark Janes from comment #30)
> > (In reply to Mark Janes from comment #28)
> > >   https://android-review.googlesource.com/c/platform/external/deqp/+/901894
> > 
> > Mesa still asserts with this fix.  I also tested Andrii's mesa patch with
> > the dEQP fix and the test fails.
> Do you mean the Chris's dEQP fix here, yes?
> But looks like the mentioned Chris's dEQP fix considers some GL limitations
> and doesn't affect the expectations of binding points.
> 
> Also the assertion is a separate issue, I created the piglit test for that:
> https://patchwork.freedesktop.org/patch/286287/
> But yes, we unable to fix the test fail without assertion because of crash
> :-)
> 
> > 
> > Since non-mesa drivers have found issues with the original dEQP change, I
> > suspect there are still deeper problems with the test.
> Possible they have the same issue with binding points mismatch after
> optimizations by glsl compiler. 
> They could try this fix/hack for deqp which is already helped us:
> https://github.com/asimiklit/deqp/commit/
> 91cff8150944213f6da533e281ee76d95ca00f21
> If it helps them we will know that it is a common issue and it could
> expedite this:
> https://github.com/KhronosGroup/OpenGL-API/issues/46

So we have an answer from Piers Daniell:
  "I believe all buffer binding points should be consumed, regardless whether 
   the array elements are used or not. This would be the behavior of least 
   surprise to the developer. I didn't see any language that would indicate 
   that unused elements should not be counted when assigning the element to 
   the buffer binding point."

Looks like according to that current mesa implementation is mistaken and dEQP test is right.
So my dEQP "fix" should migrate to mesa somehow.
Are Ilia and Ian agree it?
Is nobody against if I investigate and try to fix it?

Seems like the the binding point increment is done somewhere here:
https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/compiler/glsl/link_uniform_blocks.cpp#L279-310
Comment 33 Ian Romanick 2019-02-20 19:54:59 UTC
(In reply to andrii simiklit from comment #32)
> (In reply to andrii simiklit from comment #31)
> > (In reply to Mark Janes from comment #30)
> > > (In reply to Mark Janes from comment #28)
> > > >   https://android-review.googlesource.com/c/platform/external/deqp/+/901894
> > > 
> > > Mesa still asserts with this fix.  I also tested Andrii's mesa patch with
> > > the dEQP fix and the test fails.
> > Do you mean the Chris's dEQP fix here, yes?
> > But looks like the mentioned Chris's dEQP fix considers some GL limitations
> > and doesn't affect the expectations of binding points.
> > 
> > Also the assertion is a separate issue, I created the piglit test for that:
> > https://patchwork.freedesktop.org/patch/286287/
> > But yes, we unable to fix the test fail without assertion because of crash
> > :-)
> > 
> > > 
> > > Since non-mesa drivers have found issues with the original dEQP change, I
> > > suspect there are still deeper problems with the test.
> > Possible they have the same issue with binding points mismatch after
> > optimizations by glsl compiler. 
> > They could try this fix/hack for deqp which is already helped us:
> > https://github.com/asimiklit/deqp/commit/
> > 91cff8150944213f6da533e281ee76d95ca00f21
> > If it helps them we will know that it is a common issue and it could
> > expedite this:
> > https://github.com/KhronosGroup/OpenGL-API/issues/46
> 
> So we have an answer from Piers Daniell:
>   "I believe all buffer binding points should be consumed, regardless
> whether 
>    the array elements are used or not. This would be the behavior of least 
>    surprise to the developer. I didn't see any language that would indicate 
>    that unused elements should not be counted when assigning the element to 
>    the buffer binding point."

I think this basically agrees with my earlier sentiment that we shouldn't trim elements from the beginning of the array.  It's generally ok (and in some cases expected) to trim elements from the end.
Comment 34 asimiklit 2019-02-21 12:14:34 UTC
(In reply to asimiklit from comment #26)
> (In reply to Ian Romanick from comment #17)
> > (In reply to asimiklit from comment #6)
> > > Created attachment 143288 [details]
> > > this simple program helps me to reproduce this issue.
> > > 
> > > just share my simple reproducer)
> > > 
> > > Run it in this way:
> > > 
> > >    simple_reproducer shader.comp
> > 
> > It seems like this could be made into a piglit test that could be compiled
> > with glslparsertest.  If you haven't already submitted such a test, please
> > do. :)
> 
> I work on it)
The two piglit tests were suggested:
https://patchwork.freedesktop.org/patch/286287/
Comment 35 asimiklit 2019-02-21 15:38:03 UTC
Created attachment 143430 [details] [review]
patch to disallow an elimination of the first unused ub/ssbo array elements

(In reply to Ian Romanick from comment #33)
> (In reply to andrii simiklit from comment #32)
> > (In reply to andrii simiklit from comment #31)
> > > (In reply to Mark Janes from comment #30)
> > > > (In reply to Mark Janes from comment #28)
> > > > >   https://android-review.googlesource.com/c/platform/external/deqp/+/901894
> > > > 
> > > > Mesa still asserts with this fix.  I also tested Andrii's mesa patch with
> > > > the dEQP fix and the test fails.
> > > Do you mean the Chris's dEQP fix here, yes?
> > > But looks like the mentioned Chris's dEQP fix considers some GL limitations
> > > and doesn't affect the expectations of binding points.
> > > 
> > > Also the assertion is a separate issue, I created the piglit test for that:
> > > https://patchwork.freedesktop.org/patch/286287/
> > > But yes, we unable to fix the test fail without assertion because of crash
> > > :-)
> > > 
> > > > 
> > > > Since non-mesa drivers have found issues with the original dEQP change, I
> > > > suspect there are still deeper problems with the test.
> > > Possible they have the same issue with binding points mismatch after
> > > optimizations by glsl compiler. 
> > > They could try this fix/hack for deqp which is already helped us:
> > > https://github.com/asimiklit/deqp/commit/
> > > 91cff8150944213f6da533e281ee76d95ca00f21
> > > If it helps them we will know that it is a common issue and it could
> > > expedite this:
> > > https://github.com/KhronosGroup/OpenGL-API/issues/46
> > 
> > So we have an answer from Piers Daniell:
> >   "I believe all buffer binding points should be consumed, regardless
> > whether 
> >    the array elements are used or not. This would be the behavior of least 
> >    surprise to the developer. I didn't see any language that would indicate 
> >    that unused elements should not be counted when assigning the element to 
> >    the buffer binding point."
> 
> I think this basically agrees with my earlier sentiment that we shouldn't
> trim elements from the beginning of the array.  It's generally ok (and in
> some cases expected) to trim elements from the end.

Are you talking about something like attached variant?


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.