rsandifo-arm added inline comments.
================ Comment at: clang/include/clang/AST/Type.h:1897 + /// 'arm_sve_vector_bits' type attribute as VectorType. + QualType getFixedLengthSveEltType(const ASTContext &Ctx) const; + ---------------- It feels to me like the information is more general than that, since the type of the element is meaningful independently of the new attribute. How about just getSveEltType instead? ================ Comment at: clang/lib/AST/ASTContext.cpp:1941 + // Adjust the alignment for fixed-length SVE predicates. + if (VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector) + Align = 16; ---------------- 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. ================ Comment at: clang/lib/AST/TextNodeDumper.cpp:1413 + case VectorType::SveFixedLengthPredicateVector: + OS << " fixed-length sve"; } ---------------- Is it worth distinguishing these, similarly to altivec? ================ Comment at: clang/lib/AST/Type.cpp:2338 + case BuiltinType::SveUint32: + return Ctx.UnsignedIntTy; + case BuiltinType::SveInt64: ---------------- 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. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:8934 + ASTContext &Context) { + auto IsValidCast = [](QualType LHSType, QualType RHSType, + ASTContext &Context) { ---------------- 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. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:8942 + VT->getVectorKind() == VectorType::SveFixedLengthPredicateVector && + isVector(RHSType, LHSType->getFixedLengthSveEltType(Context))) + return true; ---------------- Is the isVector just being defensive? I'd have expected it to be redundant, since we shouldn't create SveFixedLengthPredicateVectors with invalid element types. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:8945 + + if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector && + isVector(RHSType, LHSType->getFixedLengthSveEltType(Context))) ---------------- 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. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:9968 + if ((LHSVecType && + ((LHSVecType->getVectorKind() == VectorType::SveFixedLengthDataVector) || + (LHSVecType->getVectorKind() == ---------------- Seems like it might be useful to have a helper function for testing this pair of vector kinds. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85736/new/ https://reviews.llvm.org/D85736 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits