[PATCH] D91262: [AArch64][SVE] Allow C-style casts between fixed-size and scalable vectors

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2217-2218
 
-  // Allow reinterpret_casts between vectors of the same size and
-  // between vectors and integers of the same size.
   bool destIsVector = DestType->isVectorType();
----------------
nit: not sure we need to change this


================
Comment at: clang/lib/Sema/SemaCast.cpp:2763-2767
+  if (SrcType->isVectorType() || DestType->isVectorType())
+    if (Self.isValidSveBitcast(SrcType, DestType)) {
+      Kind = CK_BitCast;
+      return;
+    }
----------------
I think braces are recommended on the outer if as well, see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Although I suppose it could also be written as:

```
if ((SrcType->isVectorType() || DestType->isVectorType()) &&
    Self.isValidSveBitcast(SrcType, DestType)) {
  Kind = CK_BitCast;
  return;
}```


================
Comment at: clang/lib/Sema/SemaCast.cpp:2222-2227
+    // Allow bitcasting if either the source or destination is a scalable
+    // vector.
+    if (SrcType->isSizelessBuiltinType() || DestType->isSizelessBuiltinType()) {
+      Kind = CK_BitCast;
+      return TC_Success;
+    }
----------------
joechrisellis wrote:
> c-rhodes wrote:
> > This is a bit ambiguous, it'll allow bitcasting between things like a fixed predicate and any sizeless type which I don't think makes sense. I think it'll also allow bitcasting between any scalable and GNU vectors regardless of vector length, e.g.:
> > 
> > ```
> > typedef int32_t int32x4_t __attribute__((vector_size(16)));
> > 
> > void foo() {
> > svint32_t s32;
> > int32x4_t g32;
> > g32 = (int32x4_t)s32;
> > s32 = (svint32_t)g32;
> > }
> > ```
> > 
> > the above should only work when `-msve-vector-bits=128` but will currently be allowed for any N.
> Ah, you're dead right. I think the next diff should fix this, but if there are any more inconsistencies/ambiguities I'll fix them too. :) 
> Ah, you're dead right. I think the next diff should fix this, but if there are any more inconsistencies/ambiguities I'll fix them too. :) 

I suppose it's outside of the scope of this ticket, but I think we'll still need to address support for explicit conversions between scalable vectors and GNU vectors like in the example I gave.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7209-7210
+  auto ValidScalableConversion = [](QualType FirstType, QualType SecondType) {
+    if (!FirstType->getAs<BuiltinType>())
+      return false;
+
----------------
Missing a check for `isSizelessBuiltinType`


================
Comment at: clang/test/SemaCXX/aarch64-sve-explicit-casts-fixed-size.cpp:22-68
+#define TESTCASE(from, to) \
+    void from##_to_##to() {\
+        from a;            \
+        to b;              \
+                           \
+        b = (to) a;        \
+    }
----------------
nit: could simplify this a little further by wrapping the macro, e.g. (moving existing `TESTCASE` to `CAST`):
```#define TESTCASE(ty1, ty2) \
    CAST(ty1, ty2)         \
    CAST(ty2, ty1)```


================
Comment at: clang/test/SemaCXX/aarch64-sve-explicit-casts-fixed-size.cpp:24-25
+    void from##_to_##to() {\
+        from a;            \
+        to b;              \
+                           \
----------------
nit: can just pass these as args `void from##_to_##to(from a, to b) {\`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91262