[Mesa-dev] [PATCH v2 2/2] glsl/linker: location aliasing requires types to have the same width
Iago Toral
itoral at igalia.com
Tue Nov 7 07:53:48 UTC 2017
That was my intent, although it seems my patch doesn't actually
effectively prevent this for types that are not structs since the way
the code is structured we need to mark non-numerical types as consuming
all location slots to ensure that we catch aliasing attempts (which is
what is being done for structs).
At any rate, thinking a bit more about this, I think it makes sense to
allow these on contexts with bindless support enabled. I think in that
case we want to handle images, samplers and regular 64-bit ints as
variables with a numerical type of 64-bit integer and let them share
location space.
I'll send a v3 to allow this and also fix my patch to more effectively
prevent location aliasing of non-numerical types.
Thanks ilia!
Iago
On Mon, 2017-11-06 at 09:51 -0500, Ilia Mirkin wrote:
> So are we saying that you can't have explicit components on a
> bindless
> sampler/image varying, which are defined as 64-bit integer types?
>
> On Mon, Nov 6, 2017 at 7:22 AM, Iago Toral Quiroga <itoral at 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 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 (this includes records, images, etc).
> > - While we are at it, improve error reporting for the case of
> > numerical type mismatch to include the shader stage.
> > ---
> > src/compiler/glsl/link_varyings.cpp | 52
> > +++++++++++++++++++++++++++----------
> > 1 file changed, 38 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 1a9894baab..f284134c90 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -405,15 +405,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):
> > @@ -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;
> > + else
> > + return -1; /* Not a numerical type */
> > }
> >
> > static bool
> > @@ -442,7 +454,8 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> > gl_shader_stage stage)
> > {
> > unsigned last_comp;
> > - if (type->without_array()->is_record()) {
> > + const glsl_type *type_without_array = type->without_array();
> > + if (type_without_array->is_record()) {
> > /* The component qualifier can't be used on structs so just
> > treat
> > * all component slots as used.
> > */
> > @@ -458,9 +471,20 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> > struct explicit_location_info *info =
> > &explicit_locations[location][comp];
> >
> > + int numerical_type =
> > get_numerical_sized_type(type_without_array);
> > +
> > if (info->var) {
> > - /* Component aliasing is not alloed */
> > - if (comp >= component && comp < last_comp) {
> > + if (numerical_type == -1) {
> > + /* 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),
> > + 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",
> > @@ -471,12 +495,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;
> > }
> > @@ -502,7 +526,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.11.0
> >
>
>
More information about the mesa-dev
mailing list