<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - ir_variable has maximum access out of bounds -- but it's not out of bounds" href="https://bugs.freedesktop.org/show_bug.cgi?id=109532#c19">Comment # 19</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - ir_variable has maximum access out of bounds -- but it's not out of bounds" href="https://bugs.freedesktop.org/show_bug.cgi?id=109532">bug 109532</a> from <a class="email" href="mailto:andrey.simiklit.1989@gmail.com" title="andrii simiklit <andrey.simiklit.1989@gmail.com>"> andrii simiklit</a> <pre>(In reply to Ian Romanick from <a href="show_bug.cgi?id=109532#c18">comment #18</a>) > (In reply to asimiklit from <a href="show_bug.cgi?id=109532#c7">comment #7</a>) > > (In reply to Ilia Mirkin from <a href="show_bug.cgi?id=109532#c2">comment #2</a>) > > > 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? <a href="https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/compiler/glsl/link_uniform_blocks.cpp#L441">https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/compiler/glsl/link_uniform_blocks.cpp#L441</a> 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: <a href="https://android.googlesource.com/platform/external/deqp/+/master/modules/gles31/functional/es31fSSBOLayoutCase.cpp#2249">https://android.googlesource.com/platform/external/deqp/+/master/modules/gles31/functional/es31fSSBOLayoutCase.cpp#2249</a> 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the QA Contact for the bug.</li> </ul> </body> </html>