On Thu, 2019-03-21 at 00:20 +1100, Timothy Arceri wrote:
> On 20/3/19 9:31 pm, Andres Gomez wrote:
> > 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 <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.
> > >
> > > 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"
> >
> > Mmmm ... I didn't notice that example before. I think it is just
> > incorrect or, rather, a typo. The inline comment says that the float
> > takes components 2 and 3. A float will count only for *1* component.
> > Hence, the intended type there is a double, in which case the aliasing
> > is allowed.
> >
> > The spec is actually very clear just some paragraphs below:
> >
> > From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:
> >
> > " Further, when location aliasing, the aliases sharing the location
> > must have the same underlying numerical type and bit
> > width (floating-point or integer, 32-bit versus 64-bit, etc.)
> > and the same auxiliary storage and interpolation
> > qualification. The one exception where component aliasing is
> > permitted is for two input variables (not block members) to a
> > vertex shader, which are allowed to have component aliasing. This
> > vertex-variable component aliasing is intended only to support
> > vertex shaders where each execution path accesses at most one
> > input per each aliased component. Implementations are permitted,
> > but not required, to generate link-time errors if they detect
> > that every path through the vertex shader executable accesses
> > multiple inputs aliased to any single component."
>
> Yeah I noticed this when reviewing the piglit tests. It does seem we
> need to fix this. However rather than a custom
> get_numerical_sized_type() function we should be able to scrap it
> completely and possibly future proof the code by using:
>
> glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead
> for the checks.
>
> e.g.
>
> info->is_integer =
> glsl_base_type_is_integer(type->without_array()->base_type);
> info->bit_size =
> glsl_base_type_get_bit_size(type->without_array()->base_type);
>
> And compare these instead.
I don't think this is a better option. Some of the reasons, as
commented in my other mail in this thread, are explained in the
previous (unfinished) review process for this patch.
Additionally, glsl_base_type_is_integer is only true for 32bit
integers. Also, only with these 2 checks, we would be failing for
images and samplers. Finally, we would end with quite a big comparison
that will have to be scattered in several points of the
check_location_aliasing function, making more difficult to understand
the logic behind it.
This is all addressed at single point, the (already pre-existent)
get_numerical_sized_type function, which makes this easier to
understand and, IMHO, more future proof.
>
> > > Your going to need more information bug number etc to convince me this
> > > change is correct.
> >
> > I'll file a bug against the specs.
> >
> > In the meanwhile, this is the expected behavior also in the CTS tests,
> > which is also confirmed with the nVIDIA blob.
> >
> > > > 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.
> > >
> > >
> > > > 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) {
> > >
> > > 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
