[PATCH] D88905: [Clang] Allow "ext_vector_type" applied to Booleans

[Simon Moll via Phabricator via cfe-commits]

simoll added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:488-489
+
+The memory representation of a dense boolean vector is the smallest fitting
+integer.  The alignment is the number of bits rounded up to the next
+power-of-two but at most the maximum vector alignment of the target.  This
----------------
rsmith wrote:
> Given that we support `ExtInt(n)` integer types for arbitrary `n`, I don't think this is clear. Perhaps "is the same as that of the lowest-rank standard integer type of at least the specified width" or some less wordy way of saying the same thing?
> 
> What happens if you ask for more vector elements than the bit width of `unsigned long long`?
> 
> Is the rule "The size and alignment are both the number of bits rounded up to the next power of two, but the alignment is at most the maximum vector alignment of the target." ?
> Is the rule "The size and alignment are both the number of bits rounded up to the next power of two, but the alignment is at most the maximum vector alignment of the target." ?

Yes and i like your description better.


================
Comment at: clang/lib/AST/TypePrinter.cpp:654
+    OS << "__attribute__((__vector_size__(" << NumVectorElems;
+    if (!T->isExtVectorBoolean()) {
+      OS << " * sizeof(";
----------------
rsmith wrote:
> If it's an `ext_vector_type`, we shouldn't be printing it as `__vector_size__`. `ExtVectorBoolean` isn't really a special case here; we should be distinguishing `ExtVectorType` from regular `VectorType` in general.
Undid this change. There is a separate function for `printExtVectorBefore` that works fine as is.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:2084-2085
                                             Dst.isVolatileQualified());
+      auto *IRStoreTy = dyn_cast<llvm::IntegerType>(Vec->getType());
+      if (IRStoreTy) {
+        auto *IRVecTy = llvm::FixedVectorType::get(
----------------
majnemer wrote:
> `if (auto *IRStoreTy = dyn_cast<llvm::IntegerType>(Vec->getType()))`
`IRStoreTy` is used here and below


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2055-2057
+      auto *PlainIntTy = llvm::VectorType::get(Builder.getIntNTy(DstElemBits),
+                                               VecSrcTy->getElementCount());
+      Src = Builder.CreateSExt(Src, PlainIntTy);
----------------
rsmith wrote:
> I don't think we should represent widening a vector of booleans to a mask vector of 0/-1 integers as a `CK_BitCast`. `CK_BitCast` is intended for cases where the bit-pattern is reinterpreted, not where it's modified.
> 
> Can we add a new cast kind for this instead?
The ext_vector_type does not support casting and we do not need it for the bool vector type - i've removed this code path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88905