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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:466
+  typedef bool bool4 __attribute__((ext_vector_type(4)));
+  // Objects of bool8 type hold 8 bits, sizeof(bool8) == 1
+
----------------
Comment talks about `bool8` but we defined the type `bool4`.


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


================
Comment at: clang/lib/AST/ASTContext.cpp:1931
+                                     : EltInfo.Width * VT->getNumElements();
+    // enforce at least byte alignment
+    Align = std::max<unsigned>(8, Width);
----------------
Nit: please ensure that comments are formatted as full sentences (beginning with a capital letter and ending with a period) here and throughout the patch.


================
Comment at: clang/lib/AST/TypePrinter.cpp:654
+    OS << "__attribute__((__vector_size__(" << NumVectorElems;
+    if (!T->isExtVectorBoolean()) {
+      OS << " * sizeof(";
----------------
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.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1690-1691
                                                bool isNontemporal) {
-  if (!CGM.getCodeGenOpts().PreserveVec3Type) {
-    // For better performance, handle vector loads differently.
-    if (Ty->isVectorType()) {
-      const llvm::Type *EltTy = Addr.getElementType();
-
-      const auto *VTy = cast<llvm::FixedVectorType>(EltTy);
-
-      // Handle vectors of size 3 like size 4 for better performance.
-      if (VTy->getNumElements() == 3) {
-
-        // Bitcast to vec4 type.
-        auto *vec4Ty = llvm::FixedVectorType::get(VTy->getElementType(), 4);
-        Address Cast = Builder.CreateElementBitCast(Addr, vec4Ty, "castToVec4");
-        // Now load value.
-        llvm::Value *V = Builder.CreateLoad(Cast, Volatile, "loadVec4");
-
-        // Shuffle vector to get vec3.
-        V = Builder.CreateShuffleVector(V, llvm::UndefValue::get(vec4Ty),
-                                        ArrayRef<int>{0, 1, 2}, "extractVec");
-        return EmitFromMemory(V, Ty);
-      }
+  const auto *ClangVecTy = Ty->getAs<VectorType>();
+  if (ClangVecTy) {
+    // Boolean vectors use `iN` as storage type
----------------



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1771-1776
+static bool isBooleanVector(QualType Ty) {
+  const auto *VecTy = Ty->getAs<VectorType>();
+  if (!VecTy)
+    return false;
+  return VecTy->isExtVectorBoolean();
+}
----------------
Consider moving `isExtVectorBoolean()` to `Type::isExtVectorBoolType()` or similar.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1843-1844
+  llvm::Type *SrcTy = Value->getType();
+  const auto *ClangVecTy = Ty->getAs<VectorType>();
+  if (ClangVecTy) {
+    auto *VecTy = dyn_cast<llvm::FixedVectorType>(SrcTy);
----------------



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2051
+      // When casting with the same element count extend this to the native
+      // result size Otw, signextend to 'i8' as an intermediary
+      unsigned DstElemBits =
----------------
"Otw"?


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2055-2057
+      auto *PlainIntTy = llvm::VectorType::get(Builder.getIntNTy(DstElemBits),
+                                               VecSrcTy->getElementCount());
+      Src = Builder.CreateSExt(Src, PlainIntTy);
----------------
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?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7256-7258
+      (HasBitVectors & srcBoolVector) ? 1 : Context.getTypeSize(srcEltTy);
+  uint64_t destEltSize =
+      (HasBitVectors & destBoolVector) ? 1 : Context.getTypeSize(destEltTy);
----------------
Please use `&&` not `&` unless you intend a bitwise operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88905