[AArch64] Add an error if SVE scalable vector types are used in a context without sve

[dmgreen]

Summary

This adds an error message if the isSVESizelessBuiltinType like __SVFloat32_t / __SVInt64_t / etc, which provide the backing for the svfloat32_t / svint64_t / etc ACLE types, are used in a function without SVE. The alternative is a crash in the backend, which is not capable of handling scalable vector types.

When SVE is available, either through a -march=..+sve option or via a target("sve") attribute, nothing should change. Without the sve feature, this patch gives an error for any function arguments, return values and variable declarations involving the scalable types. Struct/class members and global variables already give an error. (Let me know if there is anything else that should give an error. This might not be all cases, but should rule out the most obvious.) As this can be based on the current function target attributes, the error sometimes needs to be handled later than would otherwise if it was just based on the global target.

This also adds a basic initFeatureMap method for AArch64 to include a few transient sve target dependencies. This should ideally be based on the existing map of features already present in the backend, not reproduced in the target parser.

[sdesmalen]

clang/lib/Sema/SemaDecl.cpp

8584

VLSTBuiltinType is synonymous with Vector Length Specific BuiltinType, from the docs:

/// Determines if this is a sizeless type supported by the
/// 'arm_sve_vector_bits' type attribute, which can be applied to a single
/// SVE vector or predicate, excluding tuple types such as svint32x4_t.
bool isVLSTBuiltinType() const;

Additionally, this function doesn't include the tuple types (e.g. svint32x2_t), so if you want this to work for SVE types in general you'll need to create a new method for this.

[dmgreen]

Thanks - Added a isSVESizelessBuiltinType function.

[dmgreen]

Ping

[sdesmalen]

clang/test/Sema/arm-sve-target.cpp

26

Does this implementation still allow pointers to these types?

For example, it is possible to do:

void foo(struct bar *&ptrA, struct bar *&ptrB) {
    std::swap(ptrA, ptrB);
}

without having defined bar, because it's operating purely on pointers. I would expect the same behaviour for the SVE types, because it doesn't really need SVE to do this operation.

[dmgreen]

clang/test/Sema/arm-sve-target.cpp

26

It seems to be fine. It works the same as this, without any extra warnings/errors: https://godbolt.org/z/h978aMPMb

[dmgreen]

Added some pointer tests.

[sdesmalen]

clang/lib/Basic/Targets/AArch64.cpp

721

Is it possible to address the FIXME as part of this patch? I'm a bit worried about more technical debt creeping in here, as handling the feature flags is already quite complex and replicated in different places. For example, lib/Driver/ToolChains/Arch/AArch64.cpp has a function DecodeAArch64Features which attempts to do something similar.

[dmgreen]

clang/lib/Basic/Targets/AArch64.cpp

721

Ah I see. That is the kind of thing I was hoping to avoid. The backend already knows the map of target features that depend on other features, but it's not available to TargetParser at the moment. We might need to add it in the end, but the entire feature map is really a large problem that cannot be solved very quickly. DecodeAArch64Features looks like quite a small subset of the full dependencies that we'll probably need for feature mapping. See the X86 examples in the Implied features in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/X86TargetParser.cpp.

Part of this work will hopefully involve fixing how target(..) attributes work. They currently assume a X86 model, where arch=cpu and there is no cpu=... There is no way (I don't think) to base a target feature off an architecture version, which will be needed for some of the arm_neon.h intrinsics.

So a lot of this will need to change. If you consider the target features not always coming from -march options but alternatively from target(..) attributes then the right place for this is moved away from Driver and into this TargetInfo class. I have changed this slightly, to work with setFeatureEnabled as opposed to initFeatureMap. I was hoping that this would allow the code in DecodeAArch64Features to be removed, but the tests seem to be asserting that +sve2+nosve2 should enable sve, which this would apparently not handle.

[dmgreen]

Update to use setFeatureEnabled.

[dmgreen]

Rebase over recent additions. Unfortunately the changes in setFeatureEnabled are required for a number of other patches, as the tests all depend on them. My understanding from speaking to him is that @ilinpv is adding a feature map into the TargetParser, through the Function Multiversioning support in D127812. So the code here in setFeatureEnabled is intended to be temporary, but allows some other patches to move ahead.

[dmgreen]

Rebase over other test changes.

[dmgreen]

Rebase. This requires some dependencies between target features, which is being added in D127812. I would like to get this in sooner rather than later though, to not get too close to a release. There is a simple versions for sve features in this patch. @ilinpv @sdesmalen what do you think?

[ilinpv]

In D131058#3973885, @dmgreen wrote:

Rebase. This requires some dependencies between target features, which is being added in D127812. I would like to get this in sooner rather than later though, to not get too close to a release. There is a simple versions for sve features in this patch. @ilinpv @sdesmalen what do you think?

FMV patch with target feautures dependencies relanded fe5cf480ee5a

[sdesmalen]

I'm happy to accept it, but would be good if you can rebase so that the code for inferring target features is no longer necessary.

clang/include/clang/AST/Type.h

public:
   /// every type is one or the other.  Standard types are all sized;
   /// sizeless types are purely an extension.
   ///
   /// Sizeless types contain data with no specified size, alignment,
   /// or layout.
   bool isSizelessType() const;
   bool isSizelessBuiltinType() const;
 
+  /// Returns true for SVE scalable vector types.
+  bool isSVESizelessBuiltinType() const;
+
   /// Determines if this is a sizeless type supported by the
   /// 'arm_sve_vector_bits' type attribute, which can be applied to a single
   /// SVE vector or predicate, excluding tuple types such as svint32x4_t.
   bool isVLSTBuiltinType() const;
 
   /// Returns the representative type for the element of an SVE builtin type.
   /// This is used to represent fixed-length SVE vectors created with the
   /// 'arm_sve_vector_bits' type attribute as VectorType.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_invalid_sve_type : Error<
   "%0 attribute applied to non-SVE type %1">;
 def err_attribute_bad_sve_vector_size : Error<
   "invalid SVE vector size '%0', must match value set by "
   "'-msve-vector-bits' ('%1')">;
 def err_attribute_arm_feature_sve_bits_unsupported : Error<
   "%0 is only supported when '-msve-vector-bits=<bits>' is specified with a "
   "value of 128, 256, 512, 1024 or 2048.">;
+def err_sve_vector_in_non_sve_target : Error<
+  "SVE vector type %0 cannot be used in a target without sve">;
 def err_attribute_requires_positive_integer : Error<
   "%0 attribute requires a %select{positive|non-negative}1 "
   "integral compile time constant expression">;
 def err_attribute_requires_opencl_version : Error<
   "attribute %0 is supported in the OpenCL version %1%select{| onwards}2">;
 def err_invalid_branch_protection_spec : Error<
   "invalid or misplaced branch protection specification '%0'">;
 def warn_unsupported_branch_protection_spec : Warning<

clang/lib/AST/Type.cpp

default:
       return false;
     }
   }
   return false;
 }
 
 bool Type::isSizelessType() const { return isSizelessBuiltinType(); }
 
+bool Type::isSVESizelessBuiltinType() const {
+  if (const BuiltinType *BT = getAs<BuiltinType>()) {
+    switch (BT->getKind()) {
+      // SVE Types
+#define SVE_TYPE(Name, Id, SingletonId) case BuiltinType::Id:
+#include "clang/Basic/AArch64SVEACLETypes.def"
+      return true;
+    default:
+      return false;
+    }
+  }
+  return false;
+}
+
 bool Type::isVLSTBuiltinType() const {
   if (const BuiltinType *BT = getAs<BuiltinType>()) {
     switch (BT->getKind()) {
     case BuiltinType::SveInt8:
     case BuiltinType::SveInt16:
     case BuiltinType::SveInt32:
     case BuiltinType::SveInt64:
     case BuiltinType::SveUint8:

clang/lib/Basic/Targets/AArch64.cpp

void AArch64TargetInfo::setFeatureEnabled(llvm::StringMap<bool> &Features,
   if ("9" == getArchVersionString(AK))
     for (llvm::AArch64::ArchKind I = llvm::AArch64::convertV9toV8(AK);
          I != llvm::AArch64::ArchKind::INVALID; --I)
       Features[llvm::AArch64::getSubArch(I)] = Enabled;
 
   for (llvm::AArch64::ArchKind I = --AK; I != llvm::AArch64::ArchKind::INVALID;
        --I)
     Features[llvm::AArch64::getSubArch(I)] = Enabled;
+
+  // FIXME: These dependencies between architecture features would be better
+  // modelled in the TargetParser, which should happen in a later patch.
+  if (Name == "sve2" && Enabled)
+    setFeatureEnabled(Features, "sve", true);
+  else if ((Name == "sve2-bitperm" || Name == "sve2-sha3" ||
+            Name == "sve2-aes" || Name == "sve2-sm4") &&
+           Enabled)
+    setFeatureEnabled(Features, "sve2", true);
+  else if (Name == "sve" && !Enabled)
+    setFeatureEnabled(Features, "sve2", false);
+  else if (Name == "sve2" && !Enabled) {
+    setFeatureEnabled(Features, "sve2-bitperm", false);
+    setFeatureEnabled(Features, "sve2-sha3", false);
+    setFeatureEnabled(Features, "sve2-aes", false);
+    setFeatureEnabled(Features, "sve2-sm4", false);
+  }
 }
 
 bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
                                              DiagnosticsEngine &Diags) {
   FPU = FPUMode;
   HasCRC = false;
   HasAES = false;
   HasSHA2 = false;
bool AArch64TargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
   setDataLayout();
 
   return true;
 }
 
 bool AArch64TargetInfo::hasBFloat16Type() const {
   return true;
 }
 
 TargetInfo::CallingConvCheckResult
 AArch64TargetInfo::checkCallingConvention(CallingConv CC) const {
   switch (CC) {
   case CC_C:
   case CC_Swift:
   case CC_SwiftAsync:
   case CC_PreserveMost:
   case CC_PreserveAll:

clang/lib/Sema/Sema.cpp

if (IsRetTy && !TI.hasFPReturn() && (IsDouble || IsFloat)) {
           << false /*show bit size*/ << 0 << Ty << true /*return*/
           << Context.getTargetInfo().getTriple().str()) {
         if (D)
           D->setInvalidDecl();
       }
       if (D)
         targetDiag(D->getLocation(), diag::note_defined_here, FD) << D;
     }
+
+    // Don't allow SVE types in functions without a SVE target.
+    if (Ty->isSVESizelessBuiltinType() && FD && FD->hasBody()) {
+      llvm::StringMap<bool> CallerFeatureMap;
+      Context.getFunctionFeatureMap(CallerFeatureMap, FD);
+      if (!Builtin::evaluateRequiredTargetFeatures(
+          "sve", CallerFeatureMap))
+        Diag(D->getLocation(), diag::err_sve_vector_in_non_sve_target) << Ty;
+    }
   };
 
   CheckType(Ty);
   if (const auto *FPTy = dyn_cast<FunctionProtoType>(Ty)) {
     for (const auto &ParamTy : FPTy->param_types())
       CheckType(ParamTy);
     CheckType(FPTy->getReturnType(), /*IsRetTy=*/true);
   }

clang/lib/Sema/SemaDecl.cpp

void Sema::CheckVariableDeclarationType(VarDecl *NewVD) {
 
   // PPC MMA non-pointer types are not allowed as non-local variable types.
   if (Context.getTargetInfo().getTriple().isPPC64() &&
       !NewVD->isLocalVarDecl() &&
       CheckPPCMMAType(T, NewVD->getLocation())) {
     NewVD->setInvalidDecl();
     return;
   }
+
+  // Check that SVE types are only used in functions with SVE available.
+  if (T->isSVESizelessBuiltinType() && CurContext->isFunctionOrMethod()) {
+    const FunctionDecl *FD = cast<FunctionDecl>(CurContext);
+    llvm::StringMap<bool> CallerFeatureMap;
+    Context.getFunctionFeatureMap(CallerFeatureMap, FD);
+    if (!Builtin::evaluateRequiredTargetFeatures(
+        "sve", CallerFeatureMap)) {
+      Diag(NewVD->getLocation(), diag::err_sve_vector_in_non_sve_target) << T;
+      NewVD->setInvalidDecl();
+      return;
+    }
+  }
 }
 
 /// Perform semantic checking on a newly-created variable
 /// declaration.
 ///
 /// This routine performs all of the type-checking required for a
 /// variable declaration once it has been built. It is used both to
 /// check variables after they have been parsed and their declarators

clang/test/Sema/arm-sve-target.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -DNONEON -std=c++11 -triple aarch64-arm-none-eabi -target-feature -sve %s
+
+// A target without sve should not be able to use sve types.
+
+void test_var() {
+  __SVFloat32_t x; // expected-error {{SVE vector type '__SVFloat32_t' cannot be used in a target without sve}}
+}
+
+__attribute__((target("sve")))
+void test_var_target() {
+  __SVFloat32_t x;
+}
+
+__attribute__((target("sve2")))
+void test_var_target2() {
+  __SVFloat32_t x;
+}
+
+__attribute__((target("sve2-bitperm")))
+void test_var_target3() {
+  __SVFloat32_t x;
+}
+
+__SVFloat32_t other_ret();
+__SVFloat32_t test_ret() { // expected-error {{SVE vector type '__SVFloat32_t' cannot be used in a target without sve}}
+  return other_ret();
+}
+
+__attribute__((target("sve")))
+__SVFloat32_t test_ret_target() {
+  return other_ret();
+}
+
+void test_arg(__SVFloat32_t arg) { // expected-error {{SVE vector type '__SVFloat32_t' cannot be used in a target without sve}}
+}
+
+__attribute__((target("sve")))
+void test_arg_target(__SVFloat32_t arg) {
+}
+
+__clang_svint32x4_t test4x() { // expected-error {{SVE vector type '__clang_svint32x4_t' cannot be used in a target without sve}}
+  __clang_svint32x4_t x; // expected-error {{SVE vector type '__clang_svint32x4_t' cannot be used in a target without sve}}
+  return x;
+}
+
+__attribute__((target("sve")))
+__clang_svint32x4_t test4x_target() {
+  __clang_svint32x4_t x;
+  return x;
+}
+
+// Pointers are still valid to pass around.
+void foo(__SVFloat32_t *&ptrA, __SVFloat32_t* &ptrB) {
+    ptrA = ptrB;
+}
+
+__SVFloat32_t* foo(int x, __SVFloat32_t *ptrA) {
+    return ptrA;
+}
+