simoll planned changes to this revision. simoll marked 6 inline comments as done. simoll added inline comments.
================ Comment at: clang/docs/LanguageExtensions.rst:492 + +The memory representation of a boolean vector is the smallest fitting +power-of-two integer. The alignment is the alignment of that integer type. This ---------------- rsandifo-arm wrote: > It might be worth clarifying this. With the alignment referring specifically > to “integer type”, I wasn't sure what something like: > > bool __attribute__((vector_size(256))) > > would mean on targets that don't provide 256-byte integer types. Is the type > still 256-byte aligned? Not exactly: It is the alignment of the corresponding integer type. For example the following C code: typedef bool bool256 __attribute__((vector_size(256))); bool256 P; gives you (x86_64 host, no machine flags specified): %P = alloca i2048, align 32 ================ Comment at: clang/include/clang/AST/Type.h:3248 + bool isVectorSizeBoolean() const { + return (getVectorKind() == VectorKind::GenericVector) && ---------------- rsandifo-arm wrote: > Maybe isGenericBooleanVector(), so that the terminology is consistent? Just > a suggestion though, not sure if it's an improvement. I gave it this specific name so where ever it is used, it documents that whatever depends on it is specific to vector_size bool. This should be cleaned up when a better boolean vector type is introduced. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:9779 + return Ty->isBooleanType() || + (Ty->isVectorType() && + Ty->getAs<VectorType>()->getElementType()->isBooleanType()); ---------------- rsandifo-arm wrote: > Is this deliberately wider than isVectorSizeBoolean(), or does it amount to > the same thing? That's an artifact. Will narrow this down to `isVectorSizeBoolean()`. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:11929 VectorType::GenericVector); + else if (TypeSize == Context.getTypeSize(Context.BoolTy)) + return Context.getVectorType(Context.BoolTy, VTy->getNumElements(), ---------------- rsandifo-arm wrote: > In practice, won't this either be a no-op (e.g. for 4-byte-per-bool targets) > or always trump the fallback CharTy case? I wasn't sure why the case was > needed. I guess so. Will remove this case. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:11956 + /*AllowBoolConversions*/ getLangOpts().ZVector, + /*AllowBooleanOperation*/ false); if (vType.isNull()) ---------------- rsandifo-arm wrote: > Seems like it might be useful to support comparisons too (with false < true, > as for scalars). E.g. I guess x == y would otherwise need to be written ~(x > ^ y), which seems less readable. Supporting more operators would also help > in templated contexts. Agreed. ================ Comment at: clang/lib/Sema/SemaExprCXX.cpp:6271 + /*AllowBoolConversions*/ false, + /*AllowBoolOperator*/ false); ---------------- rsandifo-arm wrote: > Any reason not to support this for booleans? ?: seems like a natural > operation for all vector element types. Sure, will allow it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81083/new/ https://reviews.llvm.org/D81083 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits