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

[Joe Ellis via Phabricator via cfe-commits]

joechrisellis marked an inline comment as done.
joechrisellis added inline comments.


================
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 =
----------------
fpetrogalli wrote:
> 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.
Simplified the code as per your suggestion, but the lambda here here serves a purpose: it makes sure that `areLaxCompatibleSveTypes` has the same behaviour irrespective of the ordering of the parameters. So we do actually need the if statements inside the lambda.


================
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()) {
----------------
fpetrogalli wrote:
> This code path seems untested.
Thinking about it, this could do with being broken out into its own patch. Will do this.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:1650-1652
+      ICK = ICK_SVE_Vector_Conversion;
+      return true;
+    }
----------------
fpetrogalli wrote:
> tabs!
Not sure where these came from, since I ran `clang-format` over the patch. Think they should be gone now...


================
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;
----------------
fpetrogalli wrote:
> 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.
> ```
Will break this out into another patch as mentioned above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91067