Today the construction logic for vector types (VectorType, DependentSizedVectorType, ExtVectorType, DependentSizedExtVectorType) doesn’t consider qualifiers on the element type and simply use the element type as-is along with its canonical form.
This create some unexpected behaviors:
using FloatConst = float const;
using FloatConst2 = FloatConst __attribute__((vector_size(8)));
using Float2 = float __attribute__((vector_size(8)));
using ConstFloat2 = const Float2;
static_assert(__is_same(FloatConst2, ConstFloat2), "ERROR"); // Fails
The GCC behavior is different from the Clang behavior ( Compiler Explorer ) – i.e. FloatConst2, ConstFloat2 are fundamentally the same type.
I believe that this is just a Clang bug that got unnoticed because in most of the use cases the vector attributes are always used in type aliases to define pretty names for vector types, and in that context qualifiers are not considered.
I’m proposing to fix the logic used to compute the canonical vector types to ensure that all qualifiers on the element type are moved to the resulting vector type – i.e. a canonical vector type has always a unqualified element type.
I think the right way to approach this is to ask what the language model of vector types w.r.t. qualified element types is supposed to be. There are three options that I see:
Qualifiers on the element type are independent from qualifiers on the vector type. This is basically the behavior of qualified struct fields. Removing qualifiers from the vector type doesn’t affect the element type, which means you can never truly lose the const-ness of a vector of const elements. Various language restrictions around mutability need to be looking through vector types to make sure the element isn’t const the same way they currently look through structs.
Qualifiers on the element type are merged with qualifiers on the vector type. This is basically the behavior of arrays. The obvious implementation model here is to just move the qualifiers outwards. Just like arrays, this implies that removing a qualifier from a vector type may require rewriting the element type so that the element type can be desugared enough to remove the qualifier.
Qualifiers on the element type are forbidden.
The last option is the easiest, and frankly it seems reasonable in the abstract, but it’s probably unacceptable because of 20+ years of source compatibility. I think the second option is best, but it is quite a bit more complicated than just changing vector type canonicalization, because e.g. you need to actually diagnose conflicting qualifiers at different levels.
The logic I applied in the PR is the same as the array one. I don’t see what special handling needs to be done to handle the merge of qualifiers – this to me seems already be happening already just like it would when using typedefs:
typedef const int ConstInt;
typedef const ConstInt ConstConstInt;
// Address space handling.
typedef ConstConstInt __attribute__((address_space(1))) AS1ConstConstInt;
typedef AS1ConstConstInt __attribute__((address_space(1))) AS1AS1ConstConstInt;
typedef AS1ConstConstInt __attribute__((address_space(2))) AS2AS1ConstConstInt;
// Array with addrspace qualified element type.
typedef AS1ConstConstInt Arr3AS1ConstConstInt[3];
typedef Arr3AS1ConstConstInt __attribute__((address_space(2))) AS2Arr3AS1ConstConstInt;
// Vector with addrspace qualified element type.
typedef AS1ConstConstInt __attribute__((ext_vector_type(4))) Vec4AS1ConstConstInt;
typedef Vec4AS1ConstConstInt __attribute__((address_space(2))) AS2Vec4AS1ConstConstInt;
which results in
test.c:6:41: warning: multiple identical address spaces specified for type [-Wduplicate-decl-specifier]
6 | typedef AS1ConstConstInt __attribute__((address_space(1))) AS1AS1ConstConstInt;
| ^
test.c:7:41: error: multiple address spaces specified for type
7 | typedef AS1ConstConstInt __attribute__((address_space(2))) AS2AS1ConstConstInt;
| ^
test.c:11:45: error: multiple address spaces specified for type
11 | typedef Arr3AS1ConstConstInt __attribute__((address_space(2))) AS2Arr3AS1ConstConstInt;
| ^
test.c:15:45: error: multiple address spaces specified for type
15 | typedef Vec4AS1ConstConstInt __attribute__((address_space(2))) AS2Vec4AS1ConstConstInt;
| ^
1 warning and 3 errors generated.
Ah, it’s interesting that we apply the address space attribute early but apply the specifier qualifiers late. We could probably just fix that. But it’s good that applying the vector attribute at least duplicates the inner qualifiers outwards.
Have you done the work to desugar the element type when necessary to remove qualifiers?
Have you done the work to desugar the element type when necessary to remove qualifiers?
I’m not sure what you are referring to here. The change I made was basically adopting the same construction logic of the type used for arrays – i.e. the canonical type has the qualifiers moved outwards.
For ConstantArrayType and DependentSizedArrayType the isSugared returns false and desugar returns QualType(this, 0). The same behavior apply to *VectorType.
Is there anything I’m missing?
There are various language rules that remove qualifiers. For example, lvalue-to-rvalue conversion produces an unqualified type, and, given a pattern type like const T &, template argument deduction will bind T to a type that lacks the const qualifier. If the element type of the vector type is allowed to have const, even if just in the non-canonical form, then these features need to strip const from the element type of the vector type, which may require it to be partially desugared. Alternatively, we can force the element type of the vector to be unqualified upon construction.
Is this related to function getUnqualifiedArrayType? Would it make sense to extend (and rename) it to handle vectors as well? Or should this be handled directly in getUnqualifiedType.
Today getUnqualifiedType doesn’t take a ASTContext & parameter, but if we do need to audit all the call sites to see if vector types should be handled or not, then I wonder if it would just make sense to collapse getUnqualifiedType with getUnqualifiedArrayType into a single function.
I don’t think this is a viable option given GCC behavior (see Compiler Explorer):
typedef const int cint;
typedef int __attribute__((vector_size(16))) vec4_int;
typedef cint __attribute__((vector_size(16))) vec4_cint;
static_assert(__is_same(const vec4_int, vec4_cint), "'const vec4_int' is different than 'vec4_cint'"); // PASS
static_assert(__is_const(const vec4_int), "'const vec4_int' is not const"); // PASS
static_assert(__is_const(vec4_cint), "'vec4_cint' is not const"); // PASS
static_assert(__is_same(decltype((vec4_cint){1,2,3,4}), vec4_int), "'decltype((vec4_cint){1,2,3,4})' is not 'vec4_int'"); // PASS
static_assert(__is_same(decltype((vec4_cint){1,2,3,4}), vec4_cint), "'decltype((vec4_cint){1,2,3,4})' is not 'vec4_cint'"); // FAIL
The clang area team discussed this, and tentatively approved it pending my review. I’ve now reviewed it, and it’s approved.
I think the suggested approach of moving any qualifiers from the vector’s element type onto the vector type is the right approach: it fits into our current model in a straightforward way, and it appears to be compatible with what gcc does.
(I think the implementation still needs more work, but nothing fundamental. I’ll leave comments.)
This RFC was accepted in this comment.