[michele-scandale]

This commit changes the way vector types are constructed to ensure that qualifiers on the element type are moved to the vector type itself -- i.e. a canonical vector type has always an unqualified element type.

RFC: https://discourse.llvm.org/t/90783

[llvmorg-github-actions]

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clangir
@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-backend-powerpc
@llvm/pr-subscribers-backend-spir-v
@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-clang-tidy
@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang

Author: Michele Scandale (michele-scandale)

Changes

This commit changes the way canonical vector types are constructed to ensure that qualifiers on the element type are moved to the vector type iself -- i.e. a canonical vector type has always a unqualified element type.

RFC: https://discourse.llvm.org/t/90783

---

Full diff: https://github.com/llvm/llvm-project/pull/197624.diff

4 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+62-60)
• (modified) clang/lib/Sema/SemaTypeTraits.cpp (+4-2)
• (modified) clang/test/Sema/types.c (+12)
• (modified) clang/test/SemaCXX/vector.cpp (+14)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a0894318dbd53..2c9e852817b7a 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -4654,11 +4654,13 @@ QualType ASTContext::getVectorType(QualType vecType, unsigned NumElts,
   if (VectorType *VTP = VectorTypes.FindNodeOrInsertPos(ID, InsertPos))
     return QualType(VTP, 0);
 
-  // If the element type isn't canonical, this won't be a canonical type either,
-  // so fill in the canonical type field.
+  // If the element type isn't canonical or has qualifiers, this won't be a
+  // canonical type either, so fill in the canonical type field.
   QualType Canonical;
-  if (!vecType.isCanonical()) {
-    Canonical = getVectorType(getCanonicalType(vecType), NumElts, VecKind);
+  if (!vecType.isCanonical() || vecType.hasLocalQualifiers()) {
+    SplitQualType canonSplit = getCanonicalType(vecType).split();
+    Canonical = getVectorType(QualType(canonSplit.Ty, 0), NumElts, VecKind);
+    Canonical = getQualifiedType(Canonical, canonSplit.Quals);
 
     // Get the new insert position for the node we care about.
     VectorType *NewIP = VectorTypes.FindNodeOrInsertPos(ID, InsertPos);
@@ -4674,37 +4676,36 @@ QualType ASTContext::getVectorType(QualType vecType, unsigned NumElts,
 QualType ASTContext::getDependentVectorType(QualType VecType, Expr *SizeExpr,
                                             SourceLocation AttrLoc,
                                             VectorKind VecKind) const {
+  SplitQualType CanonVecType = getCanonicalType(VecType).split();
+
   llvm::FoldingSetNodeID ID;
-  DependentVectorType::Profile(ID, *this, getCanonicalType(VecType), SizeExpr,
-                               VecKind);
+  DependentVectorType::Profile(ID, *this, QualType(CanonVecType.Ty, 0),
+                               SizeExpr, VecKind);
   void *InsertPos = nullptr;
   DependentVectorType *Canon =
       DependentVectorTypes.FindNodeOrInsertPos(ID, InsertPos);
-  DependentVectorType *New;
 
-  if (Canon) {
-    New = new (*this, alignof(DependentVectorType)) DependentVectorType(
-        VecType, QualType(Canon, 0), SizeExpr, AttrLoc, VecKind);
-  } else {
-    QualType CanonVecTy = getCanonicalType(VecType);
-    if (CanonVecTy == VecType) {
-      New = new (*this, alignof(DependentVectorType))
-          DependentVectorType(VecType, QualType(), SizeExpr, AttrLoc, VecKind);
-
-      DependentVectorType *CanonCheck =
-          DependentVectorTypes.FindNodeOrInsertPos(ID, InsertPos);
-      assert(!CanonCheck &&
-             "Dependent-sized vector_size canonical type broken");
-      (void)CanonCheck;
-      DependentVectorTypes.InsertNode(New, InsertPos);
-    } else {
-      QualType CanonTy = getDependentVectorType(CanonVecTy, SizeExpr,
-                                                SourceLocation(), VecKind);
-      New = new (*this, alignof(DependentVectorType))
-          DependentVectorType(VecType, CanonTy, SizeExpr, AttrLoc, VecKind);
-    }
+  // If we don't have one, build one.
+  if (!Canon) {
+    Canon = new (*this, TypeAlignment) DependentVectorType(
+        QualType(CanonVecType.Ty, 0), QualType(), SizeExpr, AttrLoc, VecKind);
+    DependentVectorTypes.InsertNode(Canon, InsertPos);
+    Types.push_back(Canon);
   }
 
+  // Apply qualifiers from the element type to the vector.
+  QualType CanonTy = getQualifiedType(QualType(Canon, 0), CanonVecType.Quals);
+
+  // If we didn't need extra canonicalization for the element type or the size
+  // expression, then just use that as our result.
+  if (QualType(CanonVecType.Ty, 0) == VecType &&
+      Canon->getSizeExpr() == SizeExpr)
+    return CanonTy;
+
+  // Otherwise, we need to build a type which follows the spelling
+  // of the element type.
+  auto *New = new (*this, TypeAlignment)
+      DependentVectorType(VecType, CanonTy, SizeExpr, AttrLoc, VecKind);
   Types.push_back(New);
   return QualType(New, 0);
 }
@@ -4726,11 +4727,13 @@ QualType ASTContext::getExtVectorType(QualType vecType,
   if (VectorType *VTP = VectorTypes.FindNodeOrInsertPos(ID, InsertPos))
     return QualType(VTP, 0);
 
-  // If the element type isn't canonical, this won't be a canonical type either,
-  // so fill in the canonical type field.
+  // If the element type isn't canonical or has qualifiers, this won't be a
+  // canonical type either, so fill in the canonical type field.
   QualType Canonical;
-  if (!vecType.isCanonical()) {
-    Canonical = getExtVectorType(getCanonicalType(vecType), NumElts);
+  if (!vecType.isCanonical() || vecType.hasLocalQualifiers()) {
+    SplitQualType canonSplit = getCanonicalType(vecType).split();
+    Canonical = getExtVectorType(QualType(canonSplit.Ty, 0), NumElts);
+    Canonical = getQualifiedType(Canonical, canonSplit.Quals);
 
     // Get the new insert position for the node we care about.
     VectorType *NewIP = VectorTypes.FindNodeOrInsertPos(ID, InsertPos);
@@ -4744,42 +4747,41 @@ QualType ASTContext::getExtVectorType(QualType vecType,
 }
 
 QualType
-ASTContext::getDependentSizedExtVectorType(QualType vecType,
-                                           Expr *SizeExpr,
+ASTContext::getDependentSizedExtVectorType(QualType VecType, Expr *SizeExpr,
                                            SourceLocation AttrLoc) const {
+  assert(VecKind == VectorType::GenericVector ||
+         VecKind == VectorType::PackedVector);
+
+  SplitQualType CanonVecType = getCanonicalType(VecType).split();
+
   llvm::FoldingSetNodeID ID;
-  DependentSizedExtVectorType::Profile(ID, *this, getCanonicalType(vecType),
+  DependentSizedExtVectorType::Profile(ID, *this, QualType(CanonVecType.Ty, 0),
                                        SizeExpr);
-
   void *InsertPos = nullptr;
   DependentSizedExtVectorType *Canon
     = DependentSizedExtVectorTypes.FindNodeOrInsertPos(ID, InsertPos);
-  DependentSizedExtVectorType *New;
-  if (Canon) {
-    // We already have a canonical version of this array type; use it as
-    // the canonical type for a newly-built type.
-    New = new (*this, alignof(DependentSizedExtVectorType))
-        DependentSizedExtVectorType(vecType, QualType(Canon, 0), SizeExpr,
-                                    AttrLoc);
-  } else {
-    QualType CanonVecTy = getCanonicalType(vecType);
-    if (CanonVecTy == vecType) {
-      New = new (*this, alignof(DependentSizedExtVectorType))
-          DependentSizedExtVectorType(vecType, QualType(), SizeExpr, AttrLoc);
-
-      DependentSizedExtVectorType *CanonCheck
-        = DependentSizedExtVectorTypes.FindNodeOrInsertPos(ID, InsertPos);
-      assert(!CanonCheck && "Dependent-sized ext_vector canonical type broken");
-      (void)CanonCheck;
-      DependentSizedExtVectorTypes.InsertNode(New, InsertPos);
-    } else {
-      QualType CanonExtTy = getDependentSizedExtVectorType(CanonVecTy, SizeExpr,
-                                                           SourceLocation());
-      New = new (*this, alignof(DependentSizedExtVectorType))
-          DependentSizedExtVectorType(vecType, CanonExtTy, SizeExpr, AttrLoc);
-    }
+
+  // If we don't have one, build one.
+  if (!Canon) {
+    Canon = new (*this, TypeAlignment) DependentSizedExtVectorType(
+        QualType(CanonVecType.Ty, 0), QualType(), SizeExpr, AttrLoc);
+    DependentSizedExtVectorTypes.InsertNode(Canon, InsertPos);
+    Types.push_back(Canon);
   }
 
+  // Apply qualifiers from the element type to the vector.
+  QualType CanonTy = getQualifiedType(QualType(Canon, 0), CanonVecType.Quals);
+
+  // If we didn't need extra canonicalization for the element type or the size
+  // expression, then just use that as our result.
+  if (QualType(CanonVecType.Ty, 0) == VecType &&
+      Canon->getSizeExpr() == SizeExpr)
+    return CanonTy;
+
+  // Otherwise, we need to build a type which follows the spelling
+  // of the element type.
+  auto *New = new (*this, TypeAlignment)
+      DependentSizedExtVectorType(VecType, CanonTy, SizeExpr, AttrLoc);
   Types.push_back(New);
   return QualType(New, 0);
 }
diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index c79b3f7045ca6..ee89f5a20a289 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -1625,8 +1625,10 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT,
   case BTT_TypeCompatible: {
     // GCC ignores cv-qualifiers on arrays for this builtin.
     Qualifiers LhsQuals, RhsQuals;
-    QualType Lhs = Self.getASTContext().getUnqualifiedArrayType(LhsT, LhsQuals);
-    QualType Rhs = Self.getASTContext().getUnqualifiedArrayType(RhsT, RhsQuals);
+    QualType Lhs = Self.getASTContext().getUnqualifiedArrayType(
+        LhsT.getCanonicalType(), LhsQuals);
+    QualType Rhs = Self.getASTContext().getUnqualifiedArrayType(
+        RhsT.getCanonicalType(), RhsQuals);
     return Self.Context.typesAreCompatible(Lhs, Rhs);
   }
   case BTT_IsConvertible:
diff --git a/clang/test/Sema/types.c b/clang/test/Sema/types.c
index 2be0e6544f3d7..21ee2feabc965 100644
--- a/clang/test/Sema/types.c
+++ b/clang/test/Sema/types.c
@@ -75,6 +75,18 @@ typedef int __attribute__((ext_vector_type(0x100000000))) e2;      // expected-e
 typedef int __attribute__((vector_size((__int128_t)1 << 100))) e3; // expected-error {{vector size too large}}
 typedef int __attribute__((ext_vector_type(0))) e4;                // expected-error {{zero vector size}}
 
+typedef const int cint;
+typedef int __attribute__((vector_size(16))) vec4_int;
+typedef cint __attribute__((vector_size(16))) vec4_cint;
+_Static_assert(__builtin_types_compatible_p(vec4_int, const vec4_int), "'vec4_int' and 'const vec4_int' are not compatible");
+_Static_assert(__builtin_types_compatible_p(vec4_int, volatile vec4_int), "'vec4_int' and 'const vec4_int' are not compatible");
+_Static_assert(__builtin_types_compatible_p(const vec4_int, vec4_cint), "'const vec4_int' and 'vec4_cint' are not compatible");
+typedef int __attribute__((ext_vector_type(4))) ext_vec4_int;
+typedef cint __attribute__((ext_vector_type(4))) ext_vec4_cint;
+_Static_assert(__builtin_types_compatible_p(ext_vec4_int, const ext_vec4_int), "'ext_vec4_int' and 'const ext_vec4_int' are not compatible");
+_Static_assert(__builtin_types_compatible_p(ext_vec4_int, volatile ext_vec4_int), "'ext_vec4_int' and 'const ext_vec4_int' are not compatible");
+_Static_assert(__builtin_types_compatible_p(const ext_vec4_int, ext_vec4_cint), "'const ext_vec4_int' and 'ext_vec4_cint' are not compatible'");
+
 // no support for vector enum type
 enum { e_2 } x3 __attribute__((vector_size(64))); // expected-error {{invalid vector element type}}
 
diff --git a/clang/test/SemaCXX/vector.cpp b/clang/test/SemaCXX/vector.cpp
index 355d93a2b8cee..ef91848c7d31a 100644
--- a/clang/test/SemaCXX/vector.cpp
+++ b/clang/test/SemaCXX/vector.cpp
@@ -817,3 +817,17 @@ namespace GH173347 {
 typedef short __attribute__((__vector_size__(8))) V;
 template <int N> V test(V x) { return (x % 5) * N; }
 }
+
+namespace test_canonical_quals {
+typedef const int cint;
+typedef int __attribute__((vector_size(16))) vec4_int;
+typedef cint __attribute__((vector_size(16))) vec4_cint;
+_Static_assert(!__is_same(vec4_int, const vec4_int), "'vec4_int' and 'const vec4_int' are not different");
+_Static_assert(!__is_same(vec4_int, volatile vec4_int), "'vec4_int' and 'const vec4_int' are not different");
+_Static_assert(__is_same(const vec4_int, vec4_cint), "'const vec4_int' and 'vec4_cint' are different");
+typedef int __attribute__((ext_vector_type(4))) ext_vec4_int;
+typedef cint __attribute__((ext_vector_type(4))) ext_vec4_cint;
+_Static_assert(!__is_same(ext_vec4_int, const ext_vec4_int), "'ext_vec4_int' and 'const ext_vec4_int' are not different");
+_Static_assert(!__is_same(ext_vec4_int, volatile ext_vec4_int), "'ext_vec4_int' and 'const ext_vec4_int' are not different");
+_Static_assert(__is_same(const ext_vec4_int, ext_vec4_cint), "'const ext_vec4_int' and 'ext_vec4_cint' are different");
+} // namespace test_canonical_quals

[michele-scandale]

Changes to getDependentVectorType and getDependentSizedExtVectorType are basically an adaptation of the logic in getDependentSizedArrayType.

[github-actions]

🐧 Linux x64 Test Results

• 94482 tests passed
• 4365 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🪟 Windows x64 Test Results

• 60191 tests passed
• 1975 tests skipped

✅ The build succeeded and all tests passed.

[michele-scandale]

This change shouldn't be needed. getUnqualifiedArrayType/getUnqualifiedType should correctly handle the removal of qualifiers from the vector element type on the non-canonical representation.

[michele-scandale]

Ping

[michele-scandale]

Ping

[erichkeane]

I don't think the type having a Context is appropriate? I thought we mentioned on the other review that this should store the element type with proper qualifiers instead?

[michele-scandale]

As mentioned in #198388 (comment):

If it is acceptable to add members to VectorType (and other vector related type classes) then I'd rather just have a const ASTContext & similarly to what has been done for TypeOfExprType.

Precomputing the unqualified type seems quite odd, it's an information that might be never needed. And in terms of memory increase I would still be adding an extra sizeof(void*) in both cases. Also TypeOfExprType and TypeOfType are already doing that to implement desugar. I really don't see a difference in what I'm trying to do here.

[frederick-vs-ja]

Suggested change

	using __vec = __simd_vector<typename remove_cv<_Tp>::type, __vec_size>;
	using __vec = __simd_vector<__remove_cv_t<_Tp>, __vec_size>;

Ditto below.

[philnik777]

Given that we seemingly have to apply it everywhere I wonder whether it wouldn't be better to move it into the alias instead.

[michele-scandale]

If you have the following:

template <class _ArithmeticT, size_t _Np>
using __simd_vector __attribute__((__ext_vector_type__(_Np))) _LIBCPP_NODEBUG = __remove_cv_t<_ArithmeticT>;

then the following cannot work

template <class _Tp, size_t _Np>
inline constexpr size_t __simd_vector_size_v<__simd_vector<_Tp, _Np>> = _Np;

because the presence of __remove_cv_t breaks the template type matching. This affects also other functions.

Is it fine to have a __simd_vector_impl and implement the vector traits/helpers using __simd_vector_impl for template parameter matching purposes, and then expose __simd_vector<T, N> = __simd_vector_impl<__remove_cv_t<T>, N>?

[michele-scandale]

Ping

[michele-scandale]

Ping

cc @erichkeane, @frederick-vs-ja, @philnik777

[philnik777]

I'd like to see a conclusion to the RFC before spending more time reviewing this. (CC @AaronBallman)

[AaronBallman]

I'd like to see a conclusion to the RFC before spending more time reviewing this. (CC @AaronBallman)

It's on the Clang Area Team's schedule; I'll try to ensure we get to it at the next meeting (Thur Jul 20).

[efriedma-quic]

I don't think this is testing what you want to test... maybe use something more like:

static_assert(__builtin_types_compatible_p(vec4_cint*, const vec4_int*), "");

[michele-scandale]

Makes sense. I've updated the test and rebased the PR

[michele-scandale]

@efriedma-quic @AaronBallman Any update here?