[Mesa-dev] [PATCH v4 1/6] glsl/linker: location aliasing requires types to have the same width
Timothy Arceri
tarceri at itsqueeze.com
Wed Mar 20 13:20:24 UTC 2019
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 <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.
>>
>> 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.
>
>> 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 <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) {
>>
>> 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;
>>>
More information about the mesa-dev
mailing list