[Clang] [Sema] Reject non-power-of-2 _BitInt matrix element types
#117487
Conversation
Sirraide
added
clang:frontend
|
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesEssentially, this pr makes this ill-formed: using mat4 = _BitInt(12) [[clang::matrix_type(3, 3)]];This matches preexisting behaviour for vector types (e.g. This is currently especially problematic since we sometimes lower matrix types to LLVM array types instead, and while e.g. Full diff: https://github.com/llvm/llvm-project/pull/117487.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8bd06fadfdc984..1c0e4043bbe276 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -371,6 +371,9 @@ Non-comprehensive list of changes in this release
- ``__builtin_reduce_and`` function can now be used in constant expressions.
- ``__builtin_reduce_or`` and ``__builtin_reduce_xor`` functions can now be used in constant expressions.
+- Clang now rejects ``_BitInt`` matrix element types if the bit width is less than ``CHAR_WIDTH`` or
+ not a power of two, matching preexisting behaviour for vector types.
+
New Compiler Flags
------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index eb05a6a77978af..f049e72a6b8694 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3233,8 +3233,8 @@ def err_attribute_too_few_arguments : Error<
"%0 attribute takes at least %1 argument%s1">;
def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
def err_attribute_invalid_bitint_vector_type : Error<
- "'_BitInt' vector element width must be %select{a power of 2|"
- "at least as wide as 'CHAR_BIT'}0">;
+ "'_BitInt' %select{vector|matrix}0 element width must be %select{a power of 2|"
+ "at least as wide as 'CHAR_BIT'}1">;
def err_attribute_invalid_matrix_type : Error<"invalid matrix element type %0">;
def err_attribute_bad_neon_vector_size : Error<
"Neon vector size must be 64 or 128 bits">;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index f32edc5ac06440..06b779f5ef3aa2 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -2312,6 +2312,18 @@ QualType Sema::BuildArrayType(QualType T, ArraySizeModifier ASM,
return T;
}
+bool CheckBitIntElementType(Sema &S, SourceLocation AttrLoc,
+ const BitIntType *BIT, bool ForMatrixType = false) {
+ // Only support _BitInt elements with byte-sized power of 2 NumBits.
+ unsigned NumBits = BIT->getNumBits();
+ if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
+ S.Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
+ << ForMatrixType << (NumBits < 8);
+ return true;
+ }
+ return false;
+}
+
QualType Sema::BuildVectorType(QualType CurType, Expr *SizeExpr,
SourceLocation AttrLoc) {
// The base type must be integer (not Boolean or enumeration) or float, and
@@ -2324,15 +2336,10 @@ QualType Sema::BuildVectorType(QualType CurType, Expr *SizeExpr,
Diag(AttrLoc, diag::err_attribute_invalid_vector_type) << CurType;
return QualType();
}
- // Only support _BitInt elements with byte-sized power of 2 NumBits.
- if (const auto *BIT = CurType->getAs<BitIntType>()) {
- unsigned NumBits = BIT->getNumBits();
- if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
- Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
- << (NumBits < 8);
- return QualType();
- }
- }
+
+ if (const auto *BIT = CurType->getAs<BitIntType>();
+ BIT && CheckBitIntElementType(*this, AttrLoc, BIT))
+ return QualType();
if (SizeExpr->isTypeDependent() || SizeExpr->isValueDependent())
return Context.getDependentVectorType(CurType, SizeExpr, AttrLoc,
@@ -2402,15 +2409,9 @@ QualType Sema::BuildExtVectorType(QualType T, Expr *ArraySize,
return QualType();
}
- // Only support _BitInt elements with byte-sized power of 2 NumBits.
- if (T->isBitIntType()) {
- unsigned NumBits = T->castAs<BitIntType>()->getNumBits();
- if (!llvm::isPowerOf2_32(NumBits) || NumBits < 8) {
- Diag(AttrLoc, diag::err_attribute_invalid_bitint_vector_type)
- << (NumBits < 8);
- return QualType();
- }
- }
+ if (const auto *BIT = T->getAs<BitIntType>();
+ BIT && CheckBitIntElementType(*this, AttrLoc, BIT))
+ return QualType();
if (!ArraySize->isTypeDependent() && !ArraySize->isValueDependent()) {
std::optional<llvm::APSInt> vecSize =
@@ -2455,6 +2456,10 @@ QualType Sema::BuildMatrixType(QualType ElementTy, Expr *NumRows, Expr *NumCols,
return QualType();
}
+ if (const auto *BIT = ElementTy->getAs<BitIntType>();
+ BIT && CheckBitIntElementType(*this, AttrLoc, BIT, true))
+ return QualType();
+
if (NumRows->isTypeDependent() || NumCols->isTypeDependent() ||
NumRows->isValueDependent() || NumCols->isValueDependent())
return Context.getDependentSizedMatrixType(ElementTy, NumRows, NumCols,
diff --git a/clang/test/SemaCXX/matrix-type.cpp b/clang/test/SemaCXX/matrix-type.cpp
index af31e267fdcae8..bb7a8421ca9e37 100644
--- a/clang/test/SemaCXX/matrix-type.cpp
+++ b/clang/test/SemaCXX/matrix-type.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -pedantic -fenable-matrix -std=c++11 -verify -triple x86_64-apple-darwin %s
+// RUN: %clang_cc1 -fsyntax-only -fenable-matrix -std=c++11 -verify -triple x86_64-apple-darwin %s
using matrix_double_t = double __attribute__((matrix_type(6, 6)));
using matrix_float_t = float __attribute__((matrix_type(6, 6)));
@@ -29,3 +29,13 @@ void matrix_unsupported_element_type() {
using matrix3_t = bool __attribute__((matrix_type(1, 1))); // expected-error{{invalid matrix element type 'bool'}}
using matrix4_t = TestEnum __attribute__((matrix_type(1, 1))); // expected-error{{invalid matrix element type 'TestEnum'}}
}
+
+void matrix_unsupported_bit_int() {
+ using m1 = _BitInt(2) __attribute__((matrix_type(4, 4))); // expected-error{{'_BitInt' matrix element width must be at least as wide as 'CHAR_BIT'}}
+ using m2 = _BitInt(7) __attribute__((matrix_type(4, 4))); // expected-error{{'_BitInt' matrix element width must be at least as wide as 'CHAR_BIT'}}
+ using m3 = _BitInt(9) __attribute__((matrix_type(4, 4))); // expected-error{{'_BitInt' matrix element width must be a power of 2}}
+ using m4 = _BitInt(12) __attribute__((matrix_type(4, 4))); // expected-error{{'_BitInt' matrix element width must be a power of 2}}
+ using m5 = _BitInt(8) __attribute__((matrix_type(4, 4)));
+ using m6 = _BitInt(64) __attribute__((matrix_type(4, 4)));
+ using m7 = _BitInt(256) __attribute__((matrix_type(4, 4)));
+}
|
There was a problem hiding this comment.
It feels like that the non-power-of-2 _BitInt vectors were disabled because it wasn't known how to handle them. https://reviews.llvm.org/D133634#3793653
It probably is still so, however, does it make sense to document the new behavior?
Hopefully also, since this never worked properly, this is not a breaking change.
I’ll check and if not, I’ll add some |
|
Interestingly, it seems we don’t even have CodeGen tests for |
|
Er, well this doesn’t look right: using i8x3 = _BitInt(32) __attribute__((ext_vector_type(3)));
i8x3 v1(i8x3 a) { return a; }%a.addr = alloca <3 x i32>, align 16
%extractVec = shufflevector <3 x i32> %a, <3 x i32> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 poison>
store <4 x i32> %extractVec, ptr %a.addr, align 16We’re storing a |
|
The type of an alloca is only used to determine the number of bytes allocated; it doesn't have any other semantics. So it's fine as long as the alloca is 16 bytes. Which I think it is in the given example, because of the datalayout rules for |
Yeah, that part I know; I was mainly just wondering if there’s a platform/abi/whatever where that doesn’t work out. |
|
(For context, grep for PreserveVec3Type.) Probably the code should be defensive and check for the allocation size of the vector type, but I don't think any targets define the alignment for non-power-of-2 vectors in a way that would cause issues. |
|
For reference, as per https://clang.llvm.org/docs/MatrixTypes.html#matrix-type, the only allowed element types for matrixes are
so we should either reject all BitInts or update the spec |
Doesn’t 6.2.5p22 include 6.2.5.p5 i.e. |
That's correct, |
|
I’ve added some tests and also updated the docs to mention that non-power-of-2 |
Essentially, this pr makes this ill-formed:
This matches preexisting behaviour for vector types (e.g.
ext_vector_type), and given that LLVM IR intrinsics for matrices also take vector types, it seems like a sensible thing to do.This is currently especially problematic since we sometimes lower matrix types to LLVM array types instead, and while e.g.
[4 x i32]and<4 x i32>probably have the same similar memory layout (though I don’t think it’s sound to rely on that either, see #117486),[4 x i12]and<4 x i12>definitely don’t.