[PATCH] D126380: [clang][AArch64][SVE] Implicit conversions for vector-scalar operations

[Cullen Rhodes via Phabricator via cfe-commits]

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