On Fri, Nov 3, 2017 at 6:56 AM, Iago Toral Quiroga <ito...@igalia.com> wrote:
> 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 a record variable.
Is there a piglit for this? (Which fails without this patch?)
Oh, and you hit it because you stuck the unreachable in there?
> ---
> src/compiler/glsl/link_varyings.cpp | 41
> ++++++++++++++++++++++++++++++-------
> 1 file changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp
> b/src/compiler/glsl/link_varyings.cpp
> index af938611f4..1d17deaffe 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -413,7 +413,7 @@ struct explicit_location_info {
> };
>
> static inline unsigned
> -get_numerical_type(const glsl_type *type)
> +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):
> @@ -421,10 +421,22 @@ 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.
> */
> - 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())
> + return GLSL_TYPE_INT64;
How does a (bindless) sampler/image come across? Do they come out as
->is_integer_64? (I don't think they do.)
> +
> + unreachable("Type is not numerical");
> }
>
> static bool
> @@ -442,14 +454,17 @@ check_location_aliasing(struct explicit_location_info
> explicit_locations[][4],
> gl_shader_stage stage)
> {
> unsigned last_comp;
> + bool is_record;
> if (type->without_array()->is_record()) {
> /* The component qualifier can't be used on structs so just treat
> * all component slots as used.
> */
> last_comp = 4;
> + is_record = true;
> } else {
> unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> last_comp = component + type->without_array()->vector_elements * dmul;
> + is_record = false;
> }
>
> while (location < location_limit) {
> @@ -459,8 +474,17 @@ 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 (is_record) {
> + /* Disallow location aliasing for record variables */
> + linker_error(prog,
> + "%s shader uses explicit location on record "
> + "variable %s that generates location aliasing "
> + "for location %u, component %u\n",
> + _mesa_shader_stage_to_string(stage),
> + 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 outputs explicitly "
> "assigned to location %d and component %d\n",
> @@ -472,7 +496,7 @@ check_location_aliasing(struct explicit_location_info
> explicit_locations[][4],
> * types, interpolation and auxiliary storage
> */
> if (info->numerical_type !=
> - get_numerical_type(type->without_array())) {
> + get_numerical_sized_type(type->without_array())) {
> linker_error(prog,
> "Varyings sharing the same location must "
> "have the same underlying numerical type. "
> @@ -502,7 +526,10 @@ 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());
> + if (!is_record) {
> + info->numerical_type =
> + get_numerical_sized_type(type->without_array());
> + }
> info->interpolation = interpolation;
> info->centroid = centroid;
> info->sample = sample;
> --
> 2.11.0
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
