[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 14:38:13 UTC 2019
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"
>
>
> Your going to need more information bug number etc to convince me this
> change is correct.
>
> > 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.
I believe this is already answered in the previous (unfinished) review
process through which this patch went through. Specifically, the 2nd
review that gave place to the v3 patch. You can check it here:
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175366.html
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175573.html
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175705.html
> > 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