From GLSLang Spec 4.60 Section 4.2 Scoping: "An array implicitly sized in one shader can be explicitly sized by another shader in the same stage. If no shader in a stage has an explicit size for the array, the largest implicit size (one more than the largest index used) in that stage is used. There is no cross-stage array sizing. If there is no static access to an implicitly sized array within the stage declaring it, then the array is given a size of 1, which is relevant when the array is declared within an interface block that is shared with other stages or the application (other unused arrays might be eliminated by the optimizer)." According to the paragraph above, the following piglit test should not generate any errors as s[] would be treated as an array of size 1: [vertex shader] #version 150 #extension GL_ARB_shader_storage_buffer_object: require buffer a { vec4 s[]; vec4 a[]; } b; in vec4 piglit_vertex; out vec4 c; void main(void) { c = b.a[0]; gl_Position = piglit_vertex; } [test] link error Test: spec/arb_shader_storage_buffer_object/linker/unsized_array_member.shader_test but it does. If we convert the GLSL code to SPIR-V there are no linker errors anymore since s[] seems to have type: OpTypeArray with size 1 and the linker accepts it.
(In reply to Eleni Maria Stea from comment #0) > From GLSLang Spec 4.60 Section 4.2 Scoping: Please refer to this as the "OpenGL Shading Language specification". GLSLang is a particular implementation of the GLSL specification. > "An array implicitly sized in one shader can be explicitly sized by another > shader in the same stage. If no shader in a stage has an explicit size for > the array, the largest implicit size (one more than the largest index used) > in that stage is used. There is no cross-stage array sizing. If there is no > static access to an implicitly sized array within the stage declaring it, > then the array is given a size of 1, which is relevant when the array is > declared within an interface block that is shared with other stages or the > application (other unused arrays might be eliminated by the optimizer)." I think we should submit a spec bug for this. There is explicit language in the spec that says that only the last member of an SSBO may be declared without a size. See issue #2 in the ARB_shader_storage_buffer_object spec. > According to the paragraph above, the following piglit test should not > generate any errors as s[] would be treated as an array of size 1: > > [vertex shader] > #version 150 > #extension GL_ARB_shader_storage_buffer_object: require > buffer a { > vec4 s[]; > vec4 a[]; > } b; > > in vec4 piglit_vertex; > out vec4 c; > > void main(void) { > c = b.a[0]; > > gl_Position = piglit_vertex; > } > > [test] > link error > > Test: > spec/arb_shader_storage_buffer_object/linker/unsized_array_member.shader_test > > but it does. > > If we convert the GLSL code to SPIR-V there are no linker errors anymore > since s[] seems to have type: OpTypeArray with size 1 and the linker accepts > it.
(In reply to Ian Romanick from comment #1) > (In reply to Eleni Maria Stea from comment #0) > > "An array implicitly sized in one shader can be explicitly sized by another > > shader in the same stage. If no shader in a stage has an explicit size for > > the array, the largest implicit size (one more than the largest index used) > > in that stage is used. There is no cross-stage array sizing. If there is no > > static access to an implicitly sized array within the stage declaring it, > > then the array is given a size of 1, which is relevant when the array is > > declared within an interface block that is shared with other stages or the > > application (other unused arrays might be eliminated by the optimizer)." > > I think we should submit a spec bug for this. There is explicit language in > the spec that says that only the last member of an SSBO may be declared > without a size. See issue #2 in the ARB_shader_storage_buffer_object spec. Before we do that... does this work on other implementations? I am very surprised that this is valid, as it does not match recollection of how we designed SSBOs. I'll be a little surprised if this works on other drivers. Let's collection some information about that, then decide how to proceed. Which ever way is determined to be correct, we should also add a test for the CTS.
I really hate it, but I fear they may be allowed. From the GLSL 4.60 spec in the section on UBO and SSBO layouts: > The shared qualifier overrides only the std140, std430, and packed > qualifiers; other qualifiers are inherited. The compiler/linker will > ensure that multiple programs and programmable stages containing this > definition will share the same memory layout for this block, as long > as all arrays are declared with explicit sizes and all matrices have > matching row_major and/or column_major qualifications (which may come > from a declaration outside the block definition). I really wish I hadn't found this text and I think it's silly but there it is.
(In reply to Jason Ekstrand from comment #3) > I really hate it, but I fear they may be allowed. From the GLSL 4.60 spec > in the section on UBO and SSBO layouts: > > > The shared qualifier overrides only the std140, std430, and packed > > qualifiers; other qualifiers are inherited. The compiler/linker will > > ensure that multiple programs and programmable stages containing this > > definition will share the same memory layout for this block, as long > > as all arrays are declared with explicit sizes and all matrices have > > matching row_major and/or column_major qualifications (which may come > > from a declaration outside the block definition). > > I really wish I hadn't found this text and I think it's silly but there it > is. I don't think this text has anything to do with this bug. s[] is not declared an explicit size.
(In reply to Ian Romanick from comment #4) > I don't think this text has anything to do with this bug. s[] is not > declared an explicit size. Exactly. My point is that the spec seems to explicitly allow for implicitly sized arrays (as opposed variable-length arrays at the end of the block). I'm not sure that actually means anything but I wouldn't be surprised if someone argues that it's intended. That said, I really hope we can get it disallowed because it's utterly insane at least for explicit buffer layouts. (I could see an argument in favor if the SSBO is implicitly laid out.)
> (In reply to Jason Ekstrand from comment #3) > > I really hate it, but I fear they may be allowed. From the GLSL 4.60 spec > > in the section on UBO and SSBO layouts: > > > > > The shared qualifier overrides only the std140, std430, and packed > > > qualifiers; other qualifiers are inherited. The compiler/linker will > > > ensure that multiple programs and programmable stages containing this > > > definition will share the same memory layout for this block, as long > > > as all arrays are declared with explicit sizes and all matrices have > > > matching row_major and/or column_major qualifications (which may come > > > from a declaration outside the block definition). s[] is either illegal or it is implicitly sized as though it we s[1]. Therefore either linking fails (as we currently do) or the application gets zero guarantees about the layout of the SSBO regardless of the declared layout qualifiers because a different shader using the same declaration may implicitly size s[] to a different size. This bug is about whether s[] is a link-time error or an implicit size of 1. The quoted text above has absolutely nothing to do with that because it requires that s[] be explicitly sized.
The nvidia proprietary driver fails to link that test with the following error message: error: different buffer variables (named a.a[0] and a.s[0]) sharing the same offset within a storage block (named a) between shaders I wonder if that means it has decided a.s has zero size and therefore they occupy the same offset. Either way it looks like they aren’t following the spec either.
(In reply to Ian Romanick from comment #2) > Before we do that... does this work on other implementations? I am very > surprised that this is valid, as it does not match recollection of how we > designed SSBOs. I'll be a little surprised if this works on other drivers. > Let's collection some information about that, then decide how to proceed. I ran the test on a proprietary ATI driver [1], here's the output: $ bin/shader_runner tests/spec/arb_shader_storage_buffer_object/linker/unsized_array_member.shader_test Failed to compile VS: Vertex shader failed to compile with the following errors: ERROR: 0:5: error(#413) Invalid implict array size is used on "uniform block member", it only can be used on array's outer level ERROR: 0:4: error(#415) only last member of shader storage buffer can be unsized array ERROR: 0:13: error(#143) Undeclared identifier: b ERROR: 0:13: error(#216) Vector field selection out of range "a" ERROR: 0:13: error(#145) Left of "[" is not of type array, matrix, or vector: expression ERROR: 0:13: error(#160) Cannot convert from: "float" to: "default out highp 4-component vector of vec4" ERROR: error(#273) 6 compilation errors. No code generated [1]: OpenGL renderer string: AMD Radeon HD 7400M Series OpenGL version string: 4.4.13374 Compatibility Profile Context 13.35.1005 (installed fglrx version: 2:15.200-0ubuntu0.5)
Okay... this doesn't work on any desktop implementation of GLSL. In previous cases like this we have decided we have decided that the spec is either unclear or wrong. Either way, please submit a Khronos spec bug for this issue that included the test case and the results from all the implementations.
(In reply to Ian Romanick from comment #9) > Okay... this doesn't work on any desktop implementation of GLSL. In > previous cases like this we have decided we have decided that the spec is > either unclear or wrong. Either way, please submit a Khronos spec bug for > this issue that included the test case and the results from all the > implementations. I've just opened an issue in GLSL, thank you!
(In reply to Jason Ekstrand from comment #3) > I really hate it, but I fear they may be allowed. From the GLSL 4.60 spec > in the section on UBO and SSBO layouts: > > > The shared qualifier overrides only the std140, std430, and packed > > qualifiers; other qualifiers are inherited. The compiler/linker will > > ensure that multiple programs and programmable stages containing this > > definition will share the same memory layout for this block, as long > > as all arrays are declared with explicit sizes and all matrices have > > matching row_major and/or column_major qualifications (which may come > > from a declaration outside the block definition). > > I really wish I hadn't found this text and I think it's silly but there it > is. I think I finally understand what you were getting at here. I noticed a note in the revision history of ARB_shader_storage_buffer_objects that relates to this: Revision 9, May 7, 2012 (pbrown) - Allow the use of the .length() method on unsized and implicitly sized arrays. For unsized arrays in shader storage blocks, .length() will be computed from the size of the associated buffer object. For implicitly sized arrays, .length() will be determined at link time. Your point, I think, was that the text you quoted would only need to specifically mention explicitly sized arrays if implicitly sized arrays are also possible. I think I now also understand why all three desktop vendors implemented things this way. The same extension spec also says: (add a new paragraph to end of the section, at the bottom of p. 30) In a shader storage block, the last member may be declared without an explicit size. In this case, the effective array size is inferred at run-time from the size of the data store backing the interface block. Such unsized arrays may be indexed with general integer expressions, but may not be passed as an argument to a function or indexed with a negative constant expression. I think everyone processed that first sentence as, "In a shader storage block, ONLY the last member may be declared without an explicit size."
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/817.
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.