[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

15 ↗

(On Diff #303816)

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

15 ↗

(On Diff #303816)

Good spot! Removed.

[fpetrogalli]

Hi @joechrisellis - thank you for this patch!

I have left a couple of comments.

Francesco

clang/lib/AST/ASTContext.cpp

8584–8587

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 ↗

(On Diff #303838)

This code path seems untested.

clang/lib/Sema/SemaOverload.cpp

1650–1652

tabs!

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

19 ↗

(On Diff #303838)

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

8584–8587

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 ↗

(On Diff #303838)

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 ↗

(On Diff #303838)

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 (!FirstType->getAs<BuiltinType>())
+      return false;
+
+    const auto *VecTy = SecondType->getAs<VectorType>();
+    if (VecTy &&
+        VecTy->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 VecTy->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/SemaExpr.cpp

if (LHSType->isScalarType()) {
         ExprResult *VecExpr = &RHS;
         *VecExpr = ImpCastExprToType(VecExpr->get(), LHSType, CK_BitCast);
         Kind = CK_BitCast;
         return Compatible;
       }
     }
 
     // Allow assignments between fixed-length and sizeless SVE vectors.
-    if (((LHSType->isSizelessBuiltinType() && RHSType->isVectorType()) ||
-         (LHSType->isVectorType() && RHSType->isSizelessBuiltinType())) &&
-        Context.areCompatibleSveTypes(LHSType, RHSType)) {
+    if ((LHSType->isSizelessBuiltinType() && RHSType->isVectorType()) ||
+        (LHSType->isVectorType() && RHSType->isSizelessBuiltinType()))
+      if (Context.areCompatibleSveTypes(LHSType, RHSType) ||
+          Context.areLaxCompatibleSveTypes(LHSType, RHSType)) {
         Kind = CK_BitCast;
         return Compatible;
       }
 
     return Incompatible;
   }
 
   // Diagnose attempts to convert between __float128 and long double where
   // such conversions currently can't be handled.
   if (unsupportedTypeConversion(*this, LHSType, RHSType))
     return Incompatible;

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.c

+// 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_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}}
+}

clang/test/Sema/attr-arm-sve-vector-bits.c

 TEST_CAST_VECTOR(uint64)
 TEST_CAST_VECTOR(float16)
 TEST_CAST_VECTOR(float32)
 TEST_CAST_VECTOR(float64)
 TEST_CAST_VECTOR(bfloat16)
 TEST_CAST_COMMON(bool)
 
 // Test the implicit conversion only applies to valid types
-fixed_int8_t to_fixed_int8_t__from_svuint8_t(svuint8_t x) { return x; } // expected-error-re {{returning 'svuint8_t' (aka '__SVUint8_t') from a function with incompatible result type 'fixed_int8_t' (vector of {{[0-9]+}} 'signed char' values)}}
 fixed_bool_t to_fixed_bool_t__from_svint32_t(svint32_t x) { return x; } // expected-error-re {{returning 'svint32_t' (aka '__SVInt32_t') from a function with incompatible result type 'fixed_bool_t' (vector of {{[0-9]+}} 'unsigned char' values)}}
 
 svint64_t to_svint64_t__from_gnu_int32_t(gnu_int32_t x) { return x; } // expected-error-re {{returning 'gnu_int32_t' (vector of {{[0-9]+}} 'int32_t' values) from a function with incompatible result type 'svint64_t' (aka '__SVInt64_t')}}
 gnu_int32_t from_svint64_t__to_gnu_int32_t(svint64_t x) { return x; } // expected-error-re {{returning 'svint64_t' (aka '__SVInt64_t') from a function with incompatible result type 'gnu_int32_t' (vector of {{[0-9]+}} 'int32_t' values)}}
 
 // Test implicit conversion between SVE and GNU vector is invalid when
 // __ARM_FEATURE_SVE_BITS != N
 #if defined(__ARM_FEATURE_SVE_BITS) && __ARM_FEATURE_SVE_BITS == 512

clang/test/SemaCXX/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_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}}
+}