[AArch64][SVE] Support implicit lax vector conversions for SVE types

[joechrisellis]

Summary

Lax vector conversions was behaving incorrectly for implicit casts
between scalable and fixed-length vector types. For example, this:

#include <arm_sve.h>

#define N __ARM_FEATURE_SVE_BITS
#define FIXED_ATTR __attribute__((arm_sve_vector_bits(N)))

typedef svfloat32_t fixed_float32_t FIXED_ATTR;

void allowed_depending() {
  fixed_float32_t fs32;
  svfloat64_t s64;

  fs32 = s64;
}

... would fail because the vectors have differing lane sizes. This patch
implements the correct behaviour for
-flax-vector-conversions={none,all,integer}. Specifically:

• -flax-vector-conversions=none prevents all lax vector conversions between scalable and fixed-sized vectors.
• -flax-vector-conversions=integer allows lax vector conversions between scalable and fixed-size vectors whose element types are integers.
• -flax-vector-conversions=all allows all lax vector conversions between scalable and fixed-size vectors (including those with floating point element types).

The implicit conversions are implemented as bitcasts.

[peterwaller-arm]

fixed_float64_t appears in the commit message but also is unused.

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp

16

I can't see any uses of fixed_float64_t and fixed_int64_t?

[joechrisellis]

Address @peterwaller-arm's comment regarding unused types.

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp

16

Good spot! Removed.

[fpetrogalli]

Hi @joechrisellis - thank you for this patch!

I have left a couple of comments.

Francesco

clang/lib/AST/ASTContext.cpp

8563–8566

May I ask to avoid this triple if statement? Given that BT is not used after being defined, I think the following form would be easier to understand:

if (!FirstType->getAs<BuiltinType>())
  return false;

const auto *VT = SecondType->getAs<VectorType>();

if (VT && VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
   /// ...
    return ...
}

return false;

May I ask you to give meaningful names to the variables? BT and VT are quite cryptic to me.

Moreover.. are BT and VT really needed? You are asserting FirstType->isSizelessBuiltinType() && SecondType->isVectorType() ... the getAs calls should not fail, no? given that the lambda is local to this method, I wouldn't bother making it work for the generic case.

clang/lib/Sema/SemaCast.cpp

2222

This code path seems untested.

clang/lib/Sema/SemaOverload.cpp

1650–1652

tabs!

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp

19

Why this one in particular? To me the comment would make more sense if saying

// An explicit cast is always allowed, irrespective of the value of -flax-vector-conversions.

[joechrisellis]

Address @fpetrogalli's comments.

[joechrisellis]

clang/lib/AST/ASTContext.cpp

8563–8566

Simplified the code as per your suggestion, but the lambda here here serves a purpose: it makes sure that areLaxCompatibleSveTypes has the same behaviour irrespective of the ordering of the parameters. So we do actually need the if statements inside the lambda.

clang/lib/Sema/SemaCast.cpp

2222

Thinking about it, this could do with being broken out into its own patch. Will do this.

clang/lib/Sema/SemaOverload.cpp

1650–1652

Not sure where these came from, since I ran clang-format over the patch. Think they should be gone now...

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp

19

Will break this out into another patch as mentioned above.

[fpetrogalli]

Thank you @joechrisellis - LGTM!

[joechrisellis]

• Support C lax vector conversions.
• Test C lax vector conversions.

[joechrisellis]

Remove failing test; it was checking that a conversion _failed_, although the conversion should now _pass_ given the changes in this patch.

clang/include/clang/AST/ASTContext.h

public:
   /// types.
   bool areCompatibleVectorTypes(QualType FirstVec, QualType SecondVec);
 
   /// Return true if the given types are an SVE builtin and a VectorType that
   /// is a fixed-length representation of the SVE builtin for a specific
   /// vector-length.
   bool areCompatibleSveTypes(QualType FirstType, QualType SecondType);
 
+  /// Return true if the given vector types are lax-compatible SVE vector types,
+  /// false otherwise.
+  bool areLaxCompatibleSveTypes(QualType FirstType, QualType SecondType);
+
   /// Return true if the type has been explicitly qualified with ObjC ownership.
   /// A type may be implicitly qualified with ownership under ObjC ARC, and in
   /// some cases the compiler treats these differently.
   bool hasDirectOwnershipQualifier(QualType Ty) const;
 
   /// Return true if this is an \c NSObject object with its \c NSObject
   /// attribute set.
   static bool isObjCNSObjectType(QualType Ty) {

clang/lib/AST/ASTContext.cpp

auto IsValidCast = [this](QualType FirstType, QualType SecondType) {
     }
     return false;
   };
 
   return IsValidCast(FirstType, SecondType) ||
          IsValidCast(SecondType, FirstType);
 }
 
+bool ASTContext::areLaxCompatibleSveTypes(QualType FirstType,
+                                          QualType SecondType) {
+  assert(((FirstType->isSizelessBuiltinType() && SecondType->isVectorType()) ||
+          (FirstType->isVectorType() && SecondType->isSizelessBuiltinType())) &&
+         "Expected SVE builtin type and vector type!");
+
+  auto IsLaxCompatible = [this](QualType FirstType, QualType SecondType) {
+    if (const auto *BT = FirstType->getAs<BuiltinType>()) {
+      if (const auto *VT = SecondType->getAs<VectorType>()) {
+        if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
+          const LangOptions::LaxVectorConversionKind LVCKind =
+              getLangOpts().getLaxVectorConversions();
+
+          // If -flax-vector-conversions=all is specified, the types are
+          // certainly compatible.
+          if (LVCKind == LangOptions::LaxVectorConversionKind::All)
+            return true;
+
+          // If -flax-vector-conversions=integer is specified, the types are
+          // compatible if the elements are integer types.
+          if (LVCKind == LangOptions::LaxVectorConversionKind::Integer)
+            return VT->getElementType().getCanonicalType()->isIntegerType() &&
+                   FirstType->getSveEltType(*this)->isIntegerType();
+        }
+      }
+    }
+    return false;
+  };
+
+  return IsLaxCompatible(FirstType, SecondType) ||
+         IsLaxCompatible(SecondType, FirstType);
+}
+
 bool ASTContext::hasDirectOwnershipQualifier(QualType Ty) const {
   while (true) {
     // __strong id
     if (const AttributedType *Attr = dyn_cast<AttributedType>(Ty)) {
       if (Attr->getAttrKind() == attr::ObjCOwnership)
         return true;
 
       Ty = Attr->getModifiedType();

clang/lib/Sema/SemaCast.cpp

if (SrcType->isNullPtrType() && DestType->isIntegralType(Self.Context)) {
     return TC_Success;
   }
 
   // Allow reinterpret_casts between vectors of the same size and
   // between vectors and integers of the same size.
   bool destIsVector = DestType->isVectorType();
   bool srcIsVector = SrcType->isVectorType();
   if (srcIsVector || destIsVector) {
+    // Scalable vectors can be cast to and from liberally.
+    if (SrcType->isSizelessBuiltinType() || DestType->isSizelessBuiltinType()) {
+      Kind = CK_BitCast;
+      return TC_Success;
+    }
+
     // The non-vector type, if any, must have integral type.  This is
     // the same rule that C vector casts use; note, however, that enum
     // types are not integral in C++.
     if ((!destIsVector && !DestType->isIntegralType(Self.Context)) ||
         (!srcIsVector && !SrcType->isIntegralType(Self.Context)))
       return TC_NotApplicable;
 
     // The size we want to consider is eltCount * eltSize.

clang/lib/Sema/SemaOverload.cpp

if (ToType->isExtVectorType()) {
 
     // Vector splat from any arithmetic type to a vector.
     if (FromType->isArithmeticType()) {
       ICK = ICK_Vector_Splat;
       return true;
     }
   }
 
-  if ((ToType->isSizelessBuiltinType() || FromType->isSizelessBuiltinType()) &&
-      S.Context.areCompatibleSveTypes(FromType, ToType)) {
+  if (ToType->isSizelessBuiltinType() || FromType->isSizelessBuiltinType())
+    if (S.Context.areCompatibleSveTypes(FromType, ToType) ||
+        S.Context.areLaxCompatibleSveTypes(FromType, ToType)) {
       ICK = ICK_SVE_Vector_Conversion;
       return true;
     }
 
   // We can perform the conversion between vector types in the following cases:
   // 1)vector types are equivalent AltiVec and GCC vector types
   // 2)lax vector conversions are permitted and the vector types are of the
   //   same size
   // 3)the destination type does not have the ARM MVE strict-polymorphism
   //   attribute, which inhibits lax vector conversion for overload resolution
   //   only

clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp

+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=512 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify=lax-vector-none %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=512 -flax-vector-conversions=integer -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify=lax-vector-integer %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=512 -flax-vector-conversions=all -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify=lax-vector-all %s
+
+// lax-vector-all-no-diagnostics
+
+#include <arm_sve.h>
+
+#define N __ARM_FEATURE_SVE_BITS
+#define FIXED_ATTR __attribute__((arm_sve_vector_bits(N)))
+
+typedef svfloat32_t fixed_float32_t FIXED_ATTR;
+typedef svint32_t fixed_int32_t FIXED_ATTR;
+
+void allowed_always() {
+  fixed_float32_t ff32;
+  svfloat64_t sf64;
+
+  // This explicit cast is always allowed, irrespective of the value of -flax-vector-conversions.
+  ff32 = (fixed_float32_t)sf64;
+}
+
+void allowed_with_integer_lax_conversions() {
+  fixed_int32_t fi32;
+  svint64_t si64;
+
+  // The implicit cast here should fail if -flax-vector-conversions=none, but pass if
+  // -flax-vector-conversions={integer,all}.
+  fi32 = si64;
+  // lax-vector-none-error@-1 {{assigning to 'fixed_int32_t' (vector of 16 'int' values) from incompatible type}}
+}
+
+void allowed_with_all_lax_conversions() {
+  fixed_float32_t ff32;
+  svfloat64_t sf64;
+
+  // The implicit cast here should fail if -flax-vector-conversions={none,integer}, but pass if
+  // -flax-vector-conversions=all.
+  ff32 = sf64;
+  // lax-vector-none-error@-1 {{assigning to 'fixed_float32_t' (vector of 16 'float' values) from incompatible type}}
+  // lax-vector-integer-error@-2 {{assigning to 'fixed_float32_t' (vector of 16 'float' values) from incompatible type}}
+}