[PATCH] D91067: [AArch64][SVE] Support implicit lax vector conversions for SVE types

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli added a comment.

Hi @joechrisellis  - thank you for this patch!

I have left a couple of comments.

Francesco



================
Comment at: clang/lib/AST/ASTContext.cpp:8563-8566
+    if (const auto *BT = FirstType->getAs<BuiltinType>()) {
+      if (const auto *VT = SecondType->getAs<VectorType>()) {
+        if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
+          const LangOptions::LaxVectorConversionKind LVCKind =
----------------
May I ask to avoid this triple if statement? Given that `BT` is not used after being defined, I think the following form would be easier to understand:

```
if (!FirstType->getAs<BuiltinType>())
  return false;

const auto *VT = SecondType->getAs<VectorType>();

if (VT && VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
   /// ...
    return ...
}

return false;
```

May I ask you to give meaningful names to the variables? BT and VT are quite cryptic to me.

Moreover.. are BT and VT really needed? You are asserting `FirstType->isSizelessBuiltinType() && SecondType->isVectorType()` ... the `getAs` calls should not fail, no? given that the lambda is local to this method, I wouldn't bother making it work for the generic case.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2222
   if (srcIsVector || destIsVector) {
+    // Scalable vectors can be cast to and from liberally.
+    if (SrcType->isSizelessBuiltinType() || DestType->isSizelessBuiltinType()) {
----------------
This code path seems untested.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1650-1652
+      ICK = ICK_SVE_Vector_Conversion;
+      return true;
+    }
----------------
tabs!


================
Comment at: clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp:19
+
+  // This explicit cast is always allowed, irrespective of the value of -flax-vector-conversions.
+  ff32 = (fixed_float32_t)sf64;
----------------
Why this one in particular? To me the comment would make more sense if saying 

```
// An explicit cast is always allowed, irrespective of the value of -flax-vector-conversions.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91067