[Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width
Andres Gomez
agomez at igalia.com
Thu Apr 11 21:41:48 UTC 2019
On Tue, 2019-04-09 at 08:40 -0700, Dylan Baker wrote:
> Hi Andres,
>
> This doesn't apply cleanly to the 19.0 branch, and I'm not even sure where to
> start resolving the conflicts. If you still want this in 19.0 can you backport
> this and either create an MR against the staging/19.0 branch and mention me, or
> send a patch to the stable list and CC me? (I prefer the MR :))
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/641
The backport is not the cleanest since I had to pull also some other
change but I think it should be safe.
However, the fix is not that important so it could also be dropped.
Take a look to the (additional) changes and feel free to pick or not.
>
> Thanks,
> Dylan
>
> Quoting Andres Gomez (2019-02-01 10:05:52)
> > 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.
> >
> > 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.
> >
> > 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) {
> > + /* 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;
> > --
> > 2.20.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Br,
Andres
More information about the mesa-dev
mailing list