[Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width
Andres Gomez
agomez at igalia.com
Wed Mar 20 15:07:41 UTC 2019
On Wed, 2019-03-20 at 16:46 +0200, Andres Gomez wrote:
> On Thu, 2019-03-21 at 00:20 +1100, Timothy Arceri wrote:
> > On 20/3/19 9:31 pm, Andres Gomez wrote:
> > > On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
> > > > On 2/2/19 5:05 am, Andres Gomez wrote:
> > > > > From: Iago Toral Quiroga <itoral at igalia.com>
> > > > >
> > > > > Regarding location aliasing requirements, the OpenGL spec says:
> > > > >
> > > > > "Further, when location aliasing, the aliases sharing the location
> > > > > must have the same underlying numerical type (floating-point or
> > > > > integer)."
> > > > >
> > > > > Khronos has further clarified that this also requires the underlying
> > > > > types to have the same width, so we can't put a float and a double
> > > > > in the same location slot for example. Future versions of the spec will
> > > > > be corrected to make this clear.
> > > >
> > > > The spec has always been very clear for example that it is ok to pack a
> > > > float with a dvec3:
> > > >
> > > > From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
> > > >
> > > > "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
> > > > // and components 0 and 1 of location 9
> > > > layout(location=9, component=2) in float i; // okay, compts 2 and 3"
> > >
> > > Mmmm ... I didn't notice that example before. I think it is just
> > > incorrect or, rather, a typo. The inline comment says that the float
> > > takes components 2 and 3. A float will count only for *1* component.
> > > Hence, the intended type there is a double, in which case the aliasing
> > > is allowed.
> > >
> > > The spec is actually very clear just some paragraphs below:
> > >
> > > From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:
> > >
> > > " Further, when location aliasing, the aliases sharing the location
> > > must have the same underlying numerical type and bit
> > > width (floating-point or integer, 32-bit versus 64-bit, etc.)
> > > and the same auxiliary storage and interpolation
> > > qualification. The one exception where component aliasing is
> > > permitted is for two input variables (not block members) to a
> > > vertex shader, which are allowed to have component aliasing. This
> > > vertex-variable component aliasing is intended only to support
> > > vertex shaders where each execution path accesses at most one
> > > input per each aliased component. Implementations are permitted,
> > > but not required, to generate link-time errors if they detect
> > > that every path through the vertex shader executable accesses
> > > multiple inputs aliased to any single component."
> >
> > Yeah I noticed this when reviewing the piglit tests. It does seem we
> > need to fix this. However rather than a custom
> > get_numerical_sized_type() function we should be able to scrap it
> > completely and possibly future proof the code by using:
> >
> > glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead
> > for the checks.
> >
> > e.g.
> >
> > info->is_integer =
> > glsl_base_type_is_integer(type->without_array()->base_type);
> > info->bit_size =
> > glsl_base_type_get_bit_size(type->without_array()->base_type);
> >
> > And compare these instead.
>
> I don't think this is a better option. Some of the reasons, as
> commented in my other mail in this thread, are explained in the
> previous (unfinished) review process for this patch.
>
> Additionally, glsl_base_type_is_integer is only true for 32bit
> integers. Also, only with these 2 checks, we would be failing for
> images and samplers. Finally, we would end with quite a big comparison
> that will have to be scattered in several points of the
> check_location_aliasing function, making more difficult to understand
> the logic behind it.
Scratch the last paragraph. I committed the mistake of checking
"is_integer" instead of "glsl_base_type_is_integer"
> This is all addressed at single point, the (already pre-existent)
> get_numerical_sized_type function, which makes this easier to
> understand and, IMHO, more future proof.
Even with the possibility of using those 2, I would rather keep the
get_numerical_sized_type to address the cases of "non-numerical"
variables beyond structs.
>
> > > > Your going to need more information bug number etc to convince me this
> > > > change is correct.
> > >
> > > I'll file a bug against the specs.
> > >
> > > In the meanwhile, this is the expected behavior also in the CTS tests,
> > > which is also confirmed with the nVIDIA blob.
> > >
> > > > > This patch amends our implementation to account for this restriction.
> > > > >
> > > > > In the process of doing this, I also noticed that we would attempt
> > > > > to check aliasing requirements for record variables (including the test
> > > > > for the numerical type) which is not allowed, instead, we should be
> > > > > producing a linker error as soon as we see any attempt to do location
> > > > > aliasing on non-numerical variables. For the particular case of structs,
> > > > > we were producing a linker error in this case, but only because we
> > > > > assumed that struct fields use all components in each location, so
> > > > > any attempt to alias locations consumed by struct fields would produce
> > > > > a link error due to component aliasing, which is not accurate of the
> > > > > actual problem. This patch would make it produce an error for attempting
> > > > > to alias a non-numerical variable instead, which is always accurate.
> > > >
> > > > None of this should be needed at all. It is an error to use
> > > > location/component layouts on struct members.
> > > >
> > > > "It is a compile-time error to use a location qualifier on a member of a
> > > > structure."
> > > >
> > > > "It is a compile-time error to use *component* without also specifying
> > > > *location*"
> > > >
> > > > So depending on the component aliasing check is sufficient. If you
> > > > really want to add a better error message then see my comments below.
> > > >
> > > >
> > > > > v2:
> > > > > - Do not assert if we see invalid numerical types. These come
> > > > > straight from shader code, so we should produce linker errors if
> > > > > shaders attempt to do location aliasing on variables that are not
> > > > > numerical such as records.
> > > > > - While we are at it, improve error reporting for the case of
> > > > > numerical type mismatch to include the shader stage.
> > > > >
> > > > > v3:
> > > > > - Allow location aliasing of images and samplers. If we get these
> > > > > it means bindless support is active and they should be handled
> > > > > as 64-bit integers (Ilia)
> > > > > - Make sure we produce link errors for any non-numerical type
> > > > > for which we attempt location aliasing, not just structs.
> > > > >
> > > > > v4:
> > > > > - Rebased with minor fixes (Andres).
> > > > > - Added fixing tag to the commit log (Andres).
> > > > >
> > > > > Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> > > > > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > > > > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > > > > ---
> > > > > src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
> > > > > 1 file changed, 46 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > > > > index 3969c0120b3..3f41832ac93 100644
> > > > > --- a/src/compiler/glsl/link_varyings.cpp
> > > > > +++ b/src/compiler/glsl/link_varyings.cpp
> > > > > @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
> > > > >
> > > > > struct explicit_location_info {
> > > > > ir_variable *var;
> > > > > - unsigned numerical_type;
> > > > > + int numerical_type;
> > > > > unsigned interpolation;
> > > > > bool centroid;
> > > > > bool sample;
> > > > > bool patch;
> > > > > };
> > > > >
> > > > > -static inline unsigned
> > > > > -get_numerical_type(const glsl_type *type)
> > > > > +static inline int
> > > > > +get_numerical_sized_type(const glsl_type *type)
> > > > > {
> > > > > /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
> > > > > * (Location aliasing):
> > > > > @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
> > > > > * "Further, when location aliasing, the aliases sharing the location
> > > > > * must have the same underlying numerical type (floating-point or
> > > > > * integer)
> > > > > + *
> > > > > + * Khronos has further clarified that this also requires the underlying
> > > > > + * types to have the same width, so we can't put a float and a double
> > > > > + * in the same location slot for example. Future versions of the spec will
> > > > > + * be corrected to make this clear.
> > > > > + *
> > > > > + * Notice that we allow location aliasing for bindless image/samplers too
> > > > > + * since these are defined as 64-bit integers.
> > > > > */
> > > > > - if (type->is_float() || type->is_double())
> > > > > + if (type->is_float())
> > > > > return GLSL_TYPE_FLOAT;
> > > > > - return GLSL_TYPE_INT;
> > > > > + else if (type->is_integer())
> > > > > + return GLSL_TYPE_INT;
> > > > > + else if (type->is_double())
> > > > > + return GLSL_TYPE_DOUBLE;
> > > > > + else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> > > > > + return GLSL_TYPE_INT64;
> > > > > +
> > > > > + return -1; /* Not a numerical type */
> > > > > }
> > > > >
> > > > > static bool
> > > > > @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > > gl_shader_stage stage)
> > > > > {
> > > > > unsigned last_comp;
> > > > > - if (type->without_array()->is_record()) {
> > > > > - /* The component qualifier can't be used on structs so just treat
> > > > > - * all component slots as used.
> > > > > + const glsl_type *type_without_array = type->without_array();
> > > > > + int numerical_type = get_numerical_sized_type(type_without_array);
> > > > > + if (-1 == numerical_type) {
> > > > > + /* The component qualifier can't be used on non-numerical types so just
> > > > > + * treat all component slots as used. This will also make it so that
> > > > > + * any location aliasing attempt on non-numerical types is detected.
> > > > > */
> > > > > last_comp = 4;
> > > > > } else {
> > > > > - unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > > > > - last_comp = component + type->without_array()->vector_elements * dmul;
> > > > > + unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> > > > > + last_comp = component + type_without_array->vector_elements * dmul;
> > > > > }
> > > > >
> > > > > while (location < location_limit) {
> > > > > @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > > &explicit_locations[location][comp];
> > > > >
> > > > > if (info->var) {
> > > > > - /* Component aliasing is not alloed */
> > > > > - if (comp >= component && comp < last_comp) {
> > > > > + if (-1 == numerical_type || -1 == info->numerical_type) {
> > > >
> > > > If you really want a new message for trying to pack with a struct then
> > > > all you really need here is:
> > > >
> > > > if (info->var->type->without_array()->is_record())
> > > >
> > > > Then you can skip all this nasty -1 stuff.
> > > >
> > > > > + /* Location aliasing is only allowed for numerical variables */
> > > > > + linker_error(prog,
> > > > > + "%s shader has location aliasing for "
> > > > > + "non-numerical variable '%s'. Location %u "
> > > > > + "component %u\n",
> > > > > + _mesa_shader_stage_to_string(stage),
> > > > > + -1 == numerical_type ? var->name : info->var->name,
> > > > > + location, comp);
> > > > > + return false;
> > > > > + } else if (comp >= component && comp < last_comp) {
> > > > > + /* Component aliasing is not allowed */
> > > > > linker_error(prog,
> > > > > "%s shader has multiple %sputs explicitly "
> > > > > "assigned to location %d and component %d\n",
> > > > > @@ -491,12 +519,12 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > > /* For all other used components we need to have matching
> > > > > * types, interpolation and auxiliary storage
> > > > > */
> > > > > - if (info->numerical_type !=
> > > > > - get_numerical_type(type->without_array())) {
> > > > > + if (info->numerical_type != numerical_type) {
> > > > > linker_error(prog,
> > > > > - "Varyings sharing the same location must "
> > > > > - "have the same underlying numerical type. "
> > > > > - "Location %u component %u\n",
> > > > > + "%s shader has varyings sharing the same "
> > > > > + "location that don't have the same underlying "
> > > > > + "numerical type. Location %u component %u\n",
> > > > > + _mesa_shader_stage_to_string(stage),
> > > > > location, comp);
> > > > > return false;
> > > > > }
> > > > > @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > > }
> > > > > } else if (comp >= component && comp < last_comp) {
> > > > > info->var = var;
> > > > > - info->numerical_type = get_numerical_type(type->without_array());
> > > > > + info->numerical_type = numerical_type;
> > > > > info->interpolation = interpolation;
> > > > > info->centroid = centroid;
> > > > > info->sample = sample;
> > > > >
--
Br,
Andres
More information about the mesa-dev
mailing list