[Mesa-dev] [PATCH 3/4] glsl: apply align layout qualifier rules to block offsets
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Wed Feb 10 06:06:25 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?
> >
> > 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... >:)
>
Ian, thanks a lot for checking this :-)
Timothy, this patch is:
Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
Thanks,
Sam
> > > Sam
> > >
> > > > + member_align & (member_align - 1)) {
> > > > + _mesa_glsl_error(&loc, state, "align
> > > > layout
> > > > qualifier "
> > > > + "in not a power of 2");
> > > > + } else {
> > > > + fields[i].offset = glsl_align(offset,
> > > > member_align);
> > > > + next_offset = glsl_align(fields[i].offset
> > > > +
> > > > size, align);
> > > > + }
> > > > + }
> > > > + } else {
> > > > + fields[i].offset = glsl_align(offset,
> > > > expl_align);
> > > > + next_offset = glsl_align(fields[i].offset +
> > > > size,
> > > > align);
> > > > + }
> > > > + }
> > > > +
> > > > + if (!qual->flags.q.explicit_offset) {
> > > > if (align != 0 && size != 0)
> > > > next_offset = glsl_align(next_offset + size,
> > > > align);
> > > > }
> > > > @@ -6590,7 +6619,8 @@ ast_struct_specifier::hir(exec_list
> > > > *instructions,
> > > > ir_var_auto,
> > > > layout,
> > > > 0, /* for
> > > > interface
> > > > only */
> > > > - expl_location)
> > > > ;
> > > > + expl_location,
> > > > + 0 /* for
> > > > interface
> > > > only */);
> > > >
> > > > validate_identifier(this->name, loc, state);
> > > >
> > > > @@ -6771,6 +6801,20 @@ ast_interface_block::hir(exec_list
> > > > *instructions,
> > > > }
> > > > }
> > > >
> > > > + unsigned expl_align = 0;
> > > > + if (layout.flags.q.explicit_align) {
> > > > + if (!process_qualifier_constant(state, &loc, "align",
> > > > + layout.align,
> > > > &expl_align)) {
> > > > + return NULL;
> > > > + } else {
> > > > + if (expl_align == 0 || expl_align & (expl_align - 1))
> > > > {
> > >
> > > Same as before.
> > >
> > > Sam
> > >
> > > > + _mesa_glsl_error(&loc, state, "align layout
> > > > qualifier in
> > > > not a "
> > > > + "power of 2.");
> > > > + return NULL;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > unsigned int num_variables =
> > > > ast_process_struct_or_iface_block_members(&declared_vari
> > > > ables,
> > > > state,
> > > > @@ -6782,7 +6826,8 @@ ast_interface_block::hir(exec_list
> > > > *instructions,
> > > > var_mode,
> > > > &this->layout,
> > > > qual_stream,
> > > > - expl_location)
> > > > ;
> > > > + expl_location,
> > > > + expl_align);
> > > >
> > > > state->struct_specifier_depth--;
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
More information about the mesa-dev
mailing list