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

[Simon Moll via Phabricator via cfe-commits]

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