[Mesa-dev] [PATCH 3/4] glsl: apply align layout qualifier rules to block offsets
Timothy Arceri
timothy.arceri at collabora.com
Wed Feb 10 06:41:37 UTC 2016
On Tue, 2016-02-09 at 11:46 -0800, Ian Romanick wrote:
> On 02/09/2016 08:56 AM, Ian Romanick wrote:
> > On 02/09/2016 06:06 AM, Samuel Iglesias Gonsálvez wrote:
> > >
> > > On Tue, 2016-01-12 at 20:34 +1100, Timothy Arceri wrote:
> > > > From Section 4.4.5 (Uniform and Shader Storage Block Layout
> > > > Qualifiers) of the OpenGL 4.50 spec:
> > > >
> > > > "The align qualifier makes the start of each block member
> > > > have a
> > > > minimum byte alignment. It does not affect the internal
> > > > layout
> > > > within each member, which will still follow the std140 or
> > > > std430
> > > > rules. The specified alignment must be a power of 2, or a
> > > > compile-time error results.
> > > >
> > > > The actual alignment of a member will be the greater of the
> > > > specified align alignment and the standard (e.g., std140)
> > > > base
> > > > alignment for the member's type. The actual offset of a
> > > > member is
> > > > computed as follows: If offset was declared, start with that
> > > > offset, otherwise start with the next available offset. If
> > > > the
> > > > resulting offset is not a multiple of the actual alignment,
> > > > increase it to the first offset that is a multiple of the
> > > > actual
> > > > alignment. This results in the actual offset the member will
> > > > have.
> > > >
> > > > When align is applied to an array, it affects only the start
> > > > of
> > > > the array, not the array's internal stride. Both an offset
> > > > and an
> > > > align qualifier can be specified on a declaration.
> > > >
> > > > The align qualifier, when used on a block, has the same
> > > > effect as
> > > > qualifying each member with the same align value as declared
> > > > on
> > > > the block, and gets the same compile-time results and errors
> > > > as if
> > > > this had been done. As described in general earlier, an
> > > > individual
> > > > member can specify its own align, which overrides the block-
> > > > level
> > > > align, but just for that member.
> > > > ---
> > > > src/glsl/ast_to_hir.cpp | 51
> > > > ++++++++++++++++++++++++++++++++++++++++++++++---
> > > > 1 file changed, 48 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > > > index f5da771..fd05fd7 100644
> > > > --- a/src/glsl/ast_to_hir.cpp
> > > > +++ b/src/glsl/ast_to_hir.cpp
> > > > @@ -6212,7 +6212,8 @@
> > > > ast_process_struct_or_iface_block_members(exec_list
> > > > *instructions,
> > > > ir_variable_mode
> > > > var_mode,
> > > > ast_type_qualifier
> > > > *layout,
> > > > unsigned
> > > > block_stream,
> > > > - unsigned
> > > > expl_location)
> > > > + unsigned
> > > > expl_location,
> > > > + unsigned expl_align)
> > > > {
> > > > unsigned decl_count = 0;
> > > > unsigned next_offset = 0;
> > > > @@ -6466,6 +6467,34 @@
> > > > ast_process_struct_or_iface_block_members(exec_list
> > > > *instructions,
> > > > }
> > > > } else {
> > > > fields[i].offset = -1;
> > > > + }
> > > > +
> > > > + if (qual->flags.q.explicit_align || expl_align != 0)
> > > > {
> > > > + unsigned offset = fields[i].offset != -1 ?
> > > > fields[i].offset :
> > > > + next_offset;
> > > > + if (align == 0 || size == 0) {
> > > > + _mesa_glsl_error(&loc, state, "align can only
> > > > be used
> > > > with "
> > > > + "std430 and std140 layouts");
> > > > + } else if (qual->flags.q.explicit_align) {
> > > > + unsigned member_align;
> > > > + if (process_qualifier_constant(state, &loc,
> > > > "align",
> > > > + qual->align,
> > > > &member_align)) {
> > > > + if (member_align == 0 ||
> > >
> > > I have modified ubo-block-align-zero.vert piglit test to set the
> > > align
> > > qualifier only to block members and tested with align = 0. The
> > > shader
> > > compiles on NVIDIA proprietary driver.
> > >
> > > According to the spec, the specified alignment must be a power of
> > > 2.
> > > However align = 0 could have different interpretations (for
> > > example,
> > > when applied to a block member, it would mean to ignore block's
> > > align
> > > value). As I am not sure about this case...
> > >
> > > Have you checked if align = 0 is invalid?
Hi Sam,
Thanks for taking a look at this.
As well as the information in Ian's bug report I tested this on the AMD
driver and it also had a compile error as expected. I've also pushed a
fix for the member piglit test. It now fails to pass on Nvidia:
https://cgit.freedesktop.org/piglit/commit/?id=fc4fb04a9f9a8f6ba1ef1af9
5cfc4d8c9fc82f4a
> >
> > I looked at the ARB_enhanced_layouts spec, and it doesn't provide
> > any
> > real guidance. My gut tells me that align=0 is not valid because
> > the
> > spec doesn't say what it means. Either way, I have submitted a
> > spec bug:
> >
> > https://www.khronos.org/bugzilla/show_bug.cgi?id=1461
> >
> > Does glslang allow layout(align=0)? AMD's closed-source driver?
>
> According to https://www.khronos.org/bugzilla/show_bug.cgi?id=1461#c1
> the Khronos reference compiler does not allow align=0, so, lacking
> any
> contradictory evidence from the spec, I'm happy to not allow it
> either.
>
> Now I get to figure out how to add a conformance test for this... >:)
Hi Ian,
Since I had my AMD machine up for testing I also ran some SSO tests I
wrote a while back. The problem was after pushing them I noticed the
spec says they are not allowed, I haven't reverted them yet as I
reported this as a spec bug since its inconsistent with allowing names
to be mismatched with regular vars when they have an explicit location.
Anyway testing this on the AMD driver shows that both Nvidia and AMD
allow this behaviour so it looks like the spec should maybe be
changed.
Is this something you could follow up on if it make sense to you?
The bug is in the private bugzilla #15338
Thanks,
Tim
The tests:
https://cgit.freedesktop.org/piglit/commit/?id=f1e416b8e65ad95c6ce9e04a
80d83e7f9e1e9239
https://cgit.freedesktop.org/piglit/commit/?id=2b2bb62ce03ffa065e9b591c
d05e12c623ed4759
More information about the mesa-dev
mailing list