c-rhodes added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:13534-13556 // Strip vector types. if (isa<VectorType>(Source)) { if (Target->isVLSTBuiltinType() && (S.Context.areCompatibleSveTypes(QualType(Target, 0), QualType(Source, 0)) || S.Context.areLaxCompatibleSveTypes(QualType(Target, 0), QualType(Source, 0)))) ---------------- a lot of this is duplicated by what you've added below ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13582 + // Need the original target type for vector type checks + const Type *OriginalTarget = S.Context.getCanonicalType(T).getTypePtr(); + // Handle conversion from scalable to fixed when msve-vector-bits is ---------------- this is the same as `Target` defined on line 13473 ================ Comment at: clang/lib/Sema/SemaExpr.cpp:10267 + + if (VT) { + VectorEltTy = VT->getElementType(); ---------------- how about: ``` if (const auto *VT = VectorTy->getAs<VectorType>()) { assert(!isa<ExtVectorType>(VT) && "ExtVectorTypes should not be handled here!"); ... ``` ? ================ Comment at: clang/lib/Sema/SemaExpr.cpp:10270-10271 + } else if (VectorTy->isVLSTBuiltinType()) { + const auto *BuiltinTy = VectorTy->castAs<BuiltinType>(); + VectorEltTy = BuiltinTy->getSveEltType(S.getASTContext()); + } else { ---------------- ``` VectorEltTy = VectorTy->castAs<BuiltinType>()->getSveEltType(S.getASTContext());``` ================ Comment at: clang/lib/Sema/SemaExpr.cpp:10273 + } else { + llvm_unreachable("Only Fixed-Length and SVE Vector types are handled here"); + } ---------------- nit: fixed-length ================ Comment at: clang/test/CodeGen/aarch64-sve-vector-arith-ops.c:336 +// +svint8_t add_i8_ilit(svint8_t a) { + return a + 0; ---------------- took me a moment to realise lit -> literal, maybe add an underscore to make it clearer? I.e. `add_i8_i_lit` ================ Comment at: clang/test/Sema/aarch64-sve-vector-scalar-ops.c:1 +// RUN: %clang_cc1 -verify -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +neon -fallow-half-arguments-and-returns -fsyntax-only %s + ---------------- There's so few tests in here is it not worth add them to `clang/test/Sema/aarch64-sve-vector-arith-ops.c`? ================ Comment at: clang/test/Sema/aarch64-sve-vector-scalar-ops.c:5 + +#include <arm_neon.h> +#include <arm_sve.h> ---------------- what's NEON needed for in this test? ================ Comment at: clang/test/Sema/aarch64-sve-vector-scalar-ops.c:12 + +void add_nonscalar(svint32_t i32, struct T a32, svbool_t b) { + (void)(i32 + a32); // expected-error{{cannot convert between vector and non-scalar values ('svint32_t' (aka '__SVInt32_t') and 'struct T')}} ---------------- unused ================ Comment at: clang/test/Sema/aarch64-sve-vector-scalar-ops.c:17 +void add_bool(svbool_t b) { + (void)(b + b); // expected-error{{invalid operands to binary expression ('svbool_t' (aka '__SVBool_t') and 'svbool_t')}} +} ---------------- there's already a test for this in `clang/test/Sema/aarch64-sve-vector-arith-ops.c`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126380/new/ https://reviews.llvm.org/D126380 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits