[AArch64][SVE] Allow C-style casts between fixed-size and scalable vectors

[joechrisellis]

Summary

This patch allows C-style casting between fixed-size and scalable
vectors. This kind of cast was previously blocked by the compiler, but
it should be allowed.

[joechrisellis]

Add tests for C-style casts to/from scalable/fixed float vector types.

[fpetrogalli]

clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.cpp

1 ↗

(On Diff #304513)

Please add all cases supported, from 128 up to 2048 (doubling the size in each invocation).

16 ↗

(On Diff #304513)

Maybe write a CPP macro to reduce the amount of typing in this test? The functions below they all look the same...

clang/test/Sema/aarch64-sve-explicit-casts.cpp

5 ↗

(On Diff #304513)

Maybe use the same nomenclature of the ACLE specs, use SVE VLAT instead of just SVE.

7 ↗

(On Diff #304513)

Maybe add a link to the specs, and the version? (like in 00bet6)

[joechrisellis]

Address @fpetrogalli's comments.

• Add version of ACLE spec.
• Use ACLE nomenclature.
• Introduce preprocessor macro to reduce code size of aarch64-sve-explicit-casts.cpp.
• Test more values for -msve-vector-bits in aarch64-sve-explicit-casts.cpp.

[joechrisellis]

clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.cpp

16 ↗

(On Diff #304513)

Excellent idea, done!

[c-rhodes]

@joechrisellis thanks for the patch Joe, I've added a few comments. I also noticed this only covers C++, do we want to test C as well?

clang/lib/Sema/SemaCast.cpp

2222–2227

This is a bit ambiguous, it'll allow bitcasting between things like a fixed predicate and any sizeless type which I don't think makes sense. I think it'll also allow bitcasting between any scalable and GNU vectors regardless of vector length, e.g.:

typedef int32_t int32x4_t __attribute__((vector_size(16)));

void foo() {
svint32_t s32;
int32x4_t g32;
g32 = (int32x4_t)s32;
s32 = (svint32_t)g32;
}

the above should only work when -msve-vector-bits=128 but will currently be allowed for any N.

clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.cpp

1 ↗

(On Diff #304513)

C++ sema tests should be under clang/test/SemaCXX/

clang/test/Sema/aarch64-sve-explicit-casts.cpp

1–36 ↗

(On Diff #304513)

clang/test/SemaCXX/sizeless-1.cpp already contains a test checking C-style casts between distinct sizeless types aren't supported, I'm not sure if adding this is necessary.

[joechrisellis]

Address @c-rhodes's comments.

• Only allow casting between VLATs and VLSTs.
• Add C test.
• Move C++ test to the correct directory.
• Remove superfluous test.

clang/lib/Sema/SemaCast.cpp

2222–2227

Ah, you're dead right. I think the next diff should fix this, but if there are any more inconsistencies/ambiguities I'll fix them too. :)

clang/test/Sema/aarch64-sve-explicit-casts.cpp

1–36 ↗

(On Diff #304513)

ACK -- removed. :)

[c-rhodes]

clang/lib/Sema/SemaCast.cpp

2217–2218

nit: not sure we need to change this

2222–2227

Ah, you're dead right. I think the next diff should fix this, but if there are any more inconsistencies/ambiguities I'll fix them too. :)

I suppose it's outside of the scope of this ticket, but I think we'll still need to address support for explicit conversions between scalable vectors and GNU vectors like in the example I gave.

2763–2767

I think braces are recommended on the outer if as well, see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Although I suppose it could also be written as:

if ((SrcType->isVectorType() || DestType->isVectorType()) &&
    Self.isValidSveBitcast(SrcType, DestType)) {
  Kind = CK_BitCast;
  return;
}

clang/lib/Sema/SemaExpr.cpp

7209–7210

Missing a check for isSizelessBuiltinType

clang/test/SemaCXX/aarch64-sve-explicit-casts-fixed-size.cpp

23–69

nit: could simplify this a little further by wrapping the macro, e.g. (moving existing TESTCASE to CAST):

#define TESTCASE(ty1, ty2) \
    CAST(ty1, ty2)         \
    CAST(ty2, ty1)

25–26

nit: can just pass these as args void from##_to_##to(from a, to b) {\

[peterwaller-arm]

clang/lib/Sema/SemaCast.cpp

2222

It's good to avoid use of pronouns such as "We" in comments like this, since a different reader might take a different interpretation of the word "We". Is "We" the software itself? Is it the company who wrote the condition? Better to rewrite it in a more direct way; in my suggestion it is written so that it's clearer the following statements are what allow it. Even better if you can include a reference to a spec indicating the background on why it is allowed.

2762

Another couple of uses of the word "we". Suggest taking Cullen's suggestion to combine the conditions and say:

"Allow bitcasting between compatible SVE vector types".

2763

I don't understand why this is an || rather than an &&, please can you clarify? I would have expected they must both be vectors.

clang/lib/Sema/SemaExpr.cpp

7200

Use of "we".

[joechrisellis]

Address @c-rhodes's and @peterwaller-arm's comments.

[joechrisellis]

clang/lib/Sema/SemaCast.cpp

2217–2218

I changed this because the code changes mean that the original comment is incomplete. I.e., we now allow for reinterpret casts VLAT <-> VLST, which is not captured by the comment above.

2763

The sizeless types defined by the ACLE are represented in Clang as BuiltinTypes rather than VectorTypes. The sized types are represented as VectorTypes. The only possibility for a valid C-style cast is when we're casting VLAT <-> VLST, which is if one of the two is a VectorType. :)

[c-rhodes]

clang/lib/Sema/SemaCast.cpp

2217–2218

I changed this because the code changes mean that the original comment is incomplete. I.e., we now allow for reinterpret casts VLAT <-> VLST, which is not captured by the comment above.

It's a minor point but the reason I raised it is we have to be conscious of the fact most people probably don't care about SVE and this is changing an existing comment to put it first and foremost.

[joechrisellis]

clang/lib/Sema/SemaCast.cpp

2217–2218

Ok, I see what you mean. Will revert this part.

[joechrisellis]

Address @c-rhodes's comment regarding comment change.

[c-rhodes]

LGTM

I've left one minor comment, if that suggestion works it should be fine to fix it before committing

clang/lib/Sema/SemaExpr.cpp

7210–7212

nit: I think you can just do:

if (!FirstType->isSizelessBuiltinType())
  return false;

[joechrisellis]

Avoid doing FirstType->getAs<BuiltinType>().

clang/include/clang/Sema/Sema.h

 
   /// Type checking for matrix binary operators.
   QualType CheckMatrixElementwiseOperands(ExprResult &LHS, ExprResult &RHS,
                                           SourceLocation Loc,
                                           bool IsCompAssign);
   QualType CheckMatrixMultiplyOperands(ExprResult &LHS, ExprResult &RHS,
                                        SourceLocation Loc, bool IsCompAssign);
 
+  bool isValidSveBitcast(QualType srcType, QualType destType);
+
   bool areLaxCompatibleVectorTypes(QualType srcType, QualType destType);
   bool isLaxVectorConversion(QualType srcType, QualType destType);
 
   /// type checking declaration initializers (C99 6.7.8)
   bool CheckForConstantInitializer(Expr *e, QualType t);
 
   // type checking C++ declaration initializers (C++ [dcl.init]).
 

clang/lib/Sema/SemaCast.cpp

if (Self.Context.getTypeSize(SrcType) >
         Self.Context.getTypeSize(DestType)) {
       msg = diag::err_bad_reinterpret_cast_small_int;
       return TC_Failed;
     }
     Kind = CK_PointerToIntegral;
     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) {
+    // Allow bitcasting between SVE VLATs and VLSTs, and vice-versa.
+    if (Self.isValidSveBitcast(SrcType, DestType)) {
+      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.
void CastOperation::CheckCStyleCast() {
 
   // Allow casting a sizeless built-in type to itself.
   if (DestType->isSizelessBuiltinType() &&
       Self.Context.hasSameUnqualifiedType(DestType, SrcType)) {
     Kind = CK_NoOp;
     return;
   }
 
+  // Allow bitcasting between compatible SVE vector types.
+  if ((SrcType->isVectorType() || DestType->isVectorType()) &&
+      Self.isValidSveBitcast(SrcType, DestType)) {
+    Kind = CK_BitCast;
+    return;
+  }
+
   if (!DestType->isScalarType() && !DestType->isVectorType()) {
     const RecordType *DestRecordTy = DestType->getAs<RecordType>();
 
     if (DestRecordTy && Self.Context.hasSameUnqualifiedType(DestType, SrcType)){
       // GCC struct/union extension: allow cast to self.
       Self.Diag(OpRange.getBegin(), diag::ext_typecheck_cast_nonscalar)
         << DestType << SrcExpr.get()->getSourceRange();
       Kind = CK_NoOp;

clang/lib/Sema/SemaExpr.cpp

static bool breakDownVectorType(QualType type, uint64_t &len,
   // they're real types (i.e. non-complex, non-pointer scalar types).
   if (!type->isRealType()) return false;
 
   len = 1;
   eltType = type;
   return true;
 }
 
+/// Are the two types SVE-bitcast-compatible types? I.e. is bitcasting from the
+/// first SVE type (e.g. an SVE VLAT) to the second type (e.g. an SVE VLST)
+/// allowed?
+///
+/// This will also return false if the two given types do not make sense from
+/// the perspective of SVE bitcasts.
+bool Sema::isValidSveBitcast(QualType srcTy, QualType destTy) {
+  assert(srcTy->isVectorType() || destTy->isVectorType());
+
+  auto ValidScalableConversion = [](QualType FirstType, QualType SecondType) {
+    if (!FirstType->isSizelessBuiltinType())
+      return false;
+
+    const auto *VecTy = SecondType->getAs<VectorType>();
+    return VecTy &&
+           VecTy->getVectorKind() == VectorType::SveFixedLengthDataVector;
+  };
+
+  return ValidScalableConversion(srcTy, destTy) ||
+         ValidScalableConversion(destTy, srcTy);
+}
+
 /// Are the two types lax-compatible vector types?  That is, given
 /// that one of them is a vector, do they have equal storage sizes,
 /// where the storage size is the number of elements times the element
 /// size?
 ///
 /// This will also return false if either of the types is neither a
 /// vector nor a real type.
 bool Sema::areLaxCompatibleVectorTypes(QualType srcTy, QualType destTy) {

clang/test/Sema/aarch64-sve-explicit-casts-fixed-size.c

+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=128 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=256 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+// 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 %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=1024 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=2048 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+
+// expected-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 svfloat64_t fixed_float64_t FIXED_ATTR;
+typedef svint32_t fixed_int32_t FIXED_ATTR;
+typedef svint64_t fixed_int64_t FIXED_ATTR;
+
+// SVE VLSTs can be cast to SVE VLATs, regardless of lane size.
+// NOTE: the list below is NOT exhaustive for all SVE types.
+
+#define CAST(from, to) \
+    void from##_to_##to(from a, to b) { \
+        b = (to) a; \
+    }
+
+#define TESTCASE(ty1, ty2) \
+    CAST(ty1, ty2) \
+    CAST(ty2, ty1)
+
+TESTCASE(fixed_float32_t, svfloat32_t)
+TESTCASE(fixed_float32_t, svfloat64_t)
+TESTCASE(fixed_float32_t, svint32_t)
+TESTCASE(fixed_float32_t, svint64_t)
+
+TESTCASE(fixed_float64_t, svfloat32_t)
+TESTCASE(fixed_float64_t, svfloat64_t)
+TESTCASE(fixed_float64_t, svint32_t)
+TESTCASE(fixed_float64_t, svint64_t)
+
+TESTCASE(fixed_int32_t, svfloat32_t)
+TESTCASE(fixed_int32_t, svfloat64_t)
+TESTCASE(fixed_int32_t, svint32_t)
+TESTCASE(fixed_int32_t, svint64_t)
+
+TESTCASE(fixed_int64_t, svfloat32_t)
+TESTCASE(fixed_int64_t, svfloat64_t)
+TESTCASE(fixed_int64_t, svint32_t)
+TESTCASE(fixed_int64_t, svint64_t)

clang/test/SemaCXX/aarch64-sve-explicit-casts-fixed-size.cpp

+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=128 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=256 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+// 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 %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=1024 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -msve-vector-bits=2048 -flax-vector-conversions=none -fallow-half-arguments-and-returns -ffreestanding -fsyntax-only -verify %s
+
+// expected-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 svfloat64_t fixed_float64_t FIXED_ATTR;
+typedef svint32_t fixed_int32_t FIXED_ATTR;
+typedef svint64_t fixed_int64_t FIXED_ATTR;
+
+// SVE VLSTs can be cast to SVE VLATs, regardless of lane size.
+// NOTE: the list below is NOT exhaustive for all SVE types.
+
+#define CAST(from, to) \
+    void from##_to_##to(from a, to b) { \
+        b = (to) a; \
+    }
+
+#define TESTCASE(ty1, ty2) \
+    CAST(ty1, ty2) \
+    CAST(ty2, ty1)
+
+TESTCASE(fixed_float32_t, svfloat32_t)
+TESTCASE(fixed_float32_t, svfloat64_t)
+TESTCASE(fixed_float32_t, svint32_t)
+TESTCASE(fixed_float32_t, svint64_t)
+
+TESTCASE(fixed_float64_t, svfloat32_t)
+TESTCASE(fixed_float64_t, svfloat64_t)
+TESTCASE(fixed_float64_t, svint32_t)
+TESTCASE(fixed_float64_t, svint64_t)
+
+TESTCASE(fixed_int32_t, svfloat32_t)
+TESTCASE(fixed_int32_t, svfloat64_t)
+TESTCASE(fixed_int32_t, svint32_t)
+TESTCASE(fixed_int32_t, svint64_t)
+
+TESTCASE(fixed_int64_t, svfloat32_t)
+TESTCASE(fixed_int64_t, svfloat64_t)
+TESTCASE(fixed_int64_t, svint32_t)
+TESTCASE(fixed_int64_t, svint64_t)