[PATCH] D85736: [Sema][AArch64] Support arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked 4 inline comments as done.
c-rhodes added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:1941
+    // Adjust the alignment for fixed-length SVE predicates.
+    if (VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector)
+      Align = 16;
----------------
rsandifo-arm wrote:
> The alignment of the SVE data vectors should be 128 too (see the alignof stuff in the ACLE doc).  FWIW, there were two reasons for defining it like that:
>   - the fixed-length types map to the same ABI machine type as the variable-length types
>   - the length isn't required to be a power of 2: an implementation that supports fixed-length 384-bit vectors could define `__ARM_FEATURE_SVE_BITS` to 384.
> 
> 
> 
> The alignment of the SVE data vectors should be 128 too (see the alignof stuff in the ACLE doc).

Ah, good point. This would've bitten us when when we come to supporting non-power-of-2 vector lengths.


================
Comment at: clang/lib/AST/Type.cpp:2338
+  case BuiltinType::SveUint32:
+    return Ctx.UnsignedIntTy;
+  case BuiltinType::SveInt64:
----------------
rsandifo-arm wrote:
> This is just a note, not sure what can be done about it, but:
> 
> The element types are defined in terms of the stdint.h types.  One snag is that ILP32 newlib defines `int32_t` to be UnsignedLongTy instead of UnsignedIntTy, and defines `uint64_t` to UnsignedLongLongTy.
> 
> In GCC we got away with this because GCC already has hard-coded knowledge about the target C library's choices.  I don't think that information is available here though.
> 
> Like I say, there's nothing that necessarily needs to be “fixed”/dealt-with now, just thought it was worth mentioning.
Thanks for the heads up! I'm not familiar with ILP32 so that's good to know. So if `-mabi=ilp32` is passed then I guess we should handle that here. I'll make a note of this and address it in a later patch.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:8934
+                                  ASTContext &Context) {
+  auto IsValidCast = [](QualType LHSType, QualType RHSType,
+                        ASTContext &Context) {
----------------
rsandifo-arm wrote:
> I guess this is personal preference, but it seems more natural to use `[&]` and not pass the context.  Maybe different names from LHSType and RHSType would be better for the nested function, since it's called with both orders.
> I guess this is personal preference, but it seems more natural to use [&] and not pass the context.

Agreed, that's nicer.

> Maybe different names from LHSType and RHSType would be better for the nested function, since it's called with both orders.

I've renamed them to first/second.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:8942
+            VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector &&
+            isVector(RHSType, LHSType->getFixedLengthSveEltType(Context)))
+          return true;
----------------
rsandifo-arm wrote:
> Is the isVector just being defensive?  I'd have expected it to be redundant, since we shouldn't create SveFixedLengthPredicateVectors with invalid element types.
> Is the isVector just being defensive? I'd have expected it to be redundant, since we shouldn't create SveFixedLengthPredicateVectors with invalid element types.

Yeah although as you say it's not really necessary, fixed.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:8945
+
+        if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector &&
+            isVector(RHSType, LHSType->getFixedLengthSveEltType(Context)))
----------------
rsandifo-arm wrote:
> It looks like this could still trigger for SveBools.  Maybe it would be better to have:
> 
>   if (BT->getKind() == BuiltinType::SveBool) {
>     …
>   } else {
>     …
>   }
> 
> instead.  Perhaps it'd also be worth having an assert to show that we've already checked that any builtin type is an SVE type.
I've simplified this somewhat although I'm not sure if it's what you meant. If it's a predicate vector kind then it's compatible if the builtin is an SVE bool, otherwise it compares the types. Worth noting I've moved this to ASTContext also since it's used for the C++ conversions.


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

https://reviews.llvm.org/D85736