[PATCH] D81083: [Clang] Allow "vector_size" applied to Booleans

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a comment.

I'm not qualified to review the CodeGen stuff (or accept the patch, obvs. :-)) but FWIW, here are some comments on the doc and Sema side.

It might be good to have more Sema tests for valid and invalid usage, e.g. for which operators are valid and which aren't.

Thanks again for doing this btw.



================
Comment at: clang/docs/LanguageExtensions.rst:459
+
+Different than GCC, Clang also allows the attribute to be used with boolean
+element types. For example:
----------------
Maybe “Unlike GCC,”?  Not much in it though.


================
Comment at: clang/docs/LanguageExtensions.rst:489
+  packing).
+* Bitwise `~`, `|`, `&`, `^` and `~` are the only allowed operators on boolean
+  vectors.
----------------
`~` is listed twice.  I guess “operators” (without qualification) might make people think of the C++ keyword, in which case assignment and `[]` are allowed too.


================
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
----------------
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?


================
Comment at: clang/include/clang/AST/Type.h:3248
 
+  bool isVectorSizeBoolean() const {
+    return (getVectorKind() == VectorKind::GenericVector) &&
----------------
Maybe isGenericBooleanVector(), so that the terminology is consistent?  Just a suggestion though, not sure if it's an improvement.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9779
+  return Ty->isBooleanType() ||
+         (Ty->isVectorType() &&
+          Ty->getAs<VectorType>()->getElementType()->isBooleanType());
----------------
Is this deliberately wider than isVectorSizeBoolean(), or does it amount to the same thing?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11929
                                  VectorType::GenericVector);
+  else if (TypeSize == Context.getTypeSize(Context.BoolTy))
+    return Context.getVectorType(Context.BoolTy, VTy->getNumElements(),
----------------
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.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11956
+                          /*AllowBoolConversions*/ getLangOpts().ZVector,
+                          /*AllowBooleanOperation*/ false);
   if (vType.isNull())
----------------
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.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:6271
+                               /*AllowBoolConversions*/ false,
+                               /*AllowBoolOperator*/ false);
 
----------------
Any reason not to support this for booleans?  ?: seems like a natural operation for all vector element types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81083