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 <ito...@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 <imir...@alum.mit.edu> > > Signed-off-by: Andres Gomez <ago...@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev -- Br, Andres _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
