[PATCH] D85736: [Sema][AArch64] Support arm_sve_vector_bits attribute

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:1897
+  /// 'arm_sve_vector_bits' type attribute as VectorType.
+  QualType getFixedLengthSveEltType(const ASTContext &Ctx) const;
+
----------------
It feels to me like the information is more general than that, since the type of the element is meaningful independently of the new attribute.  How about just getSveEltType instead?


================
Comment at: clang/lib/AST/ASTContext.cpp:1941
+    // Adjust the alignment for fixed-length SVE predicates.
+    if (VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector)
+      Align = 16;
----------------
The alignment of the SVE data vectors should be 128 too (see the alignof stuff in the ACLE doc).  FWIW, there were two reasons for defining it like that:
  - the fixed-length types map to the same ABI machine type as the variable-length types
  - the length isn't required to be a power of 2: an implementation that supports fixed-length 384-bit vectors could define `__ARM_FEATURE_SVE_BITS` to 384.





================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1413
+  case VectorType::SveFixedLengthPredicateVector:
+    OS << " fixed-length sve";
   }
----------------
Is it worth distinguishing these, similarly to altivec?


================
Comment at: clang/lib/AST/Type.cpp:2338
+  case BuiltinType::SveUint32:
+    return Ctx.UnsignedIntTy;
+  case BuiltinType::SveInt64:
----------------
This is just a note, not sure what can be done about it, but:

The element types are defined in terms of the stdint.h types.  One snag is that ILP32 newlib defines `int32_t` to be UnsignedLongTy instead of UnsignedIntTy, and defines `uint64_t` to UnsignedLongLongTy.

In GCC we got away with this because GCC already has hard-coded knowledge about the target C library's choices.  I don't think that information is available here though.

Like I say, there's nothing that necessarily needs to be “fixed”/dealt-with now, just thought it was worth mentioning.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:8934
+                                  ASTContext &Context) {
+  auto IsValidCast = [](QualType LHSType, QualType RHSType,
+                        ASTContext &Context) {
----------------
I guess this is personal preference, but it seems more natural to use `[&]` and not pass the context.  Maybe different names from LHSType and RHSType would be better for the nested function, since it's called with both orders.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:8942
+            VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector &&
+            isVector(RHSType, LHSType->getFixedLengthSveEltType(Context)))
+          return true;
----------------
Is the isVector just being defensive?  I'd have expected it to be redundant, since we shouldn't create SveFixedLengthPredicateVectors with invalid element types.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:8945
+
+        if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector &&
+            isVector(RHSType, LHSType->getFixedLengthSveEltType(Context)))
----------------
It looks like this could still trigger for SveBools.  Maybe it would be better to have:

  if (BT->getKind() == BuiltinType::SveBool) {
    …
  } else {
    …
  }

instead.  Perhaps it'd also be worth having an assert to show that we've already checked that any builtin type is an SVE type.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9968
+  if ((LHSVecType &&
+       ((LHSVecType->getVectorKind() == VectorType::SveFixedLengthDataVector) ||
+        (LHSVecType->getVectorKind() ==
----------------
Seems like it might be useful to have a helper function for testing this pair of vector kinds.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85736/new/

https://reviews.llvm.org/D85736