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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some minor nits.



================
Comment at: clang/include/clang/AST/Type.h:3228
+    SveFixedLengthDataVector,
+    /// is AArch64 SVE fixed-length predicate vector
+    SveFixedLengthPredicateVector
----------------
Add a bit of vertical whitespace for consistency with surrounding enumerators?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2928
+def err_typecheck_vector_not_convertable_sizeless : Error<
+  "cannot convert between fixed-length and sizeless vector (%0 and %1)">;
 def err_typecheck_vector_not_convertable_implict_truncation : Error<
----------------
vector -> vectors
(alternatively, `a fixed-length and a sizeless vector`)


================
Comment at: clang/lib/AST/ASTContext.cpp:1945
+    // Adjust the alignment for fixed-length SVE predicates.
+    if (VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector)
+      Align = 16;
----------------
`else if`?


================
Comment at: clang/lib/AST/ASTContext.cpp:8396
+
+  auto IsValidCast = [&](QualType FirstType, QualType SecondType) {
+    if (const auto *BT = FirstType->getAs<BuiltinType>()) {
----------------
This only needs to capture `this` and not reference capture everything, right? I'd prefer that (esp given that the lambda parameters shadow the names of the function parameters).


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9913
+  // Returns true if the operands are SVE VLA and VLS types.
+  auto IsSveConversion = [](QualType &FirstType, QualType &SecondType) {
+    const VectorType *VecType = SecondType->getAs<VectorType>();
----------------
No need for the parameters to be non-const references (you can use value types here, they're cheap).


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1656
+  if (ToType->isSizelessBuiltinType() || FromType->isSizelessBuiltinType()) {
+    if (S.Context.areCompatibleSveTypes(FromType, ToType)) {
+      ICK = ICK_SVE_Vector_Conversion;
----------------
This can be hoisted into the preceding `if` statement.


================
Comment at: clang/lib/Sema/SemaType.cpp:7817
+  if (BT->getKind() == BuiltinType::SveBool) {
+    // Predicates are represented as i8
+    VecSize /= S.Context.getCharWidth() * S.Context.getCharWidth();
----------------
Full stop at the end of the comment.


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

https://reviews.llvm.org/D85736