[Matrix][Clang][HLSL] Move MaxMatrixDimension to a LangOpt

[farzonl]

fixes #160190
fixes #116710

This change just makes MaxMatrixDimension configurable by language mode. It was previously introduced in 94b4311 when there was not a need to make dimensions configurable.

Current testing to this effect exists in:

• clang/test/Sema/matrix-type-builtins.c
• clang/test/SemaCXX/matrix-type-builtins.cpp
• clang/test/SemaHLSL/BuiltIns/matrix-basic_types-errors.hlsl

New Tests to confirm configurability by language mode:

• clang/unittests/Frontend/CompilerInvocationTest.cpp

I considered adding a driver flag to clang/include/clang/Driver/Options.td but HLSL matrix max dim is always 4 so we don't need this configurable beyond that size for our use case.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Farzon Lotfi (farzonl)

Changes

fixes #160190

This change just makes MaxMatrixDimension configurable by language mode. It was previously introduced in 94b4311 when there was not a need to make dimensions configurable.

Current testing to this effect exists in:

• clang/test/Sema/matrix-type-builtins.c
• clang/test/SemaCXX/matrix-type-builtins.cpp
• clang/test/SemaHLSL/BuiltIns/matrix-basic_types-errors.hlsl

I considered adding a driver flag to clang/include/clang/Driver/Options.td but HLSL matrix max dim is always 4 so we don't need this configurable beyond that size for our use case.

---

Full diff: https://github.com/llvm/llvm-project/pull/163307.diff

7 Files Affected:

• (modified) clang/include/clang/AST/TypeBase.h (-12)
• (modified) clang/include/clang/Basic/LangOptions.def (+1)
• (modified) clang/lib/AST/ASTContext.cpp (+2-2)
• (modified) clang/lib/Basic/LangOptions.cpp (+6-2)
• (modified) clang/lib/Sema/HLSLExternalSemaSource.cpp (+2-1)
• (modified) clang/lib/Sema/SemaChecking.cpp (+2-2)
• (modified) clang/lib/Sema/SemaType.cpp (+2-2)

diff --git a/clang/include/clang/AST/TypeBase.h b/clang/include/clang/AST/TypeBase.h
index 625cc77dc1f08..5c51ec52daed3 100644
--- a/clang/include/clang/AST/TypeBase.h
+++ b/clang/include/clang/AST/TypeBase.h
@@ -4378,8 +4378,6 @@ class ConstantMatrixType final : public MatrixType {
   unsigned NumRows;
   unsigned NumColumns;
 
-  static constexpr unsigned MaxElementsPerDimension = (1 << 20) - 1;
-
   ConstantMatrixType(QualType MatrixElementType, unsigned NRows,
                      unsigned NColumns, QualType CanonElementType);
 
@@ -4398,16 +4396,6 @@ class ConstantMatrixType final : public MatrixType {
     return getNumRows() * getNumColumns();
   }
 
-  /// Returns true if \p NumElements is a valid matrix dimension.
-  static constexpr bool isDimensionValid(size_t NumElements) {
-    return NumElements > 0 && NumElements <= MaxElementsPerDimension;
-  }
-
-  /// Returns the maximum number of elements per dimension.
-  static constexpr unsigned getMaxElementsPerDimension() {
-    return MaxElementsPerDimension;
-  }
-
   void Profile(llvm::FoldingSetNodeID &ID) {
     Profile(ID, getElementType(), getNumRows(), getNumColumns(),
             getTypeClass());
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 9e850089ad87f..690439c1258c1 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -432,6 +432,7 @@ ENUM_LANGOPT(RegisterStaticDestructors, RegisterStaticDestructorsKind, 2,
 LANGOPT(RegCall4, 1, 0, NotCompatible, "Set __regcall4 as a default calling convention to respect __regcall ABI v.4")
 
 LANGOPT(MatrixTypes, 1, 0, NotCompatible, "Enable or disable the builtin matrix type")
+VALUE_LANGOPT(MaxMatrixDimension, 32, (1 << 20) - 1, NotCompatible, "maximum allowed matrix dimension")
 
 LANGOPT(CXXAssumptions, 1, 1, NotCompatible, "Enable or disable codegen and compile-time checks for C++23's [[assume]] attribute")
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index a8b41ba18fa01..fa363bc6fea7c 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -4713,8 +4713,8 @@ QualType ASTContext::getConstantMatrixType(QualType ElementTy, unsigned NumRows,
 
   assert(MatrixType::isValidElementType(ElementTy) &&
          "need a valid element type");
-  assert(ConstantMatrixType::isDimensionValid(NumRows) &&
-         ConstantMatrixType::isDimensionValid(NumColumns) &&
+  assert(NumRows > 0 && NumRows <= LangOpts.MaxMatrixDimension &&
+         NumColumns > 0 && NumColumns <= LangOpts.MaxMatrixDimension &&
          "need valid matrix dimensions");
   void *InsertPos = nullptr;
   if (ConstantMatrixType *MTP = MatrixTypes.FindNodeOrInsertPos(ID, InsertPos))
diff --git a/clang/lib/Basic/LangOptions.cpp b/clang/lib/Basic/LangOptions.cpp
index f034514466d3f..4f5d20d946de8 100644
--- a/clang/lib/Basic/LangOptions.cpp
+++ b/clang/lib/Basic/LangOptions.cpp
@@ -131,8 +131,12 @@ void LangOptions::setLangDefaults(LangOptions &Opts, Language Lang,
   Opts.NamedLoops = Std.isC2y();
 
   Opts.HLSL = Lang == Language::HLSL;
-  if (Opts.HLSL && Opts.IncludeDefaultHeader)
-    Includes.push_back("hlsl.h");
+  if (Opts.HLSL) {
+    if (Opts.IncludeDefaultHeader)
+      Includes.push_back("hlsl.h");
+    // Set maximum matrix dimension to 4 for HLSL
+    Opts.MaxMatrixDimension = 4;
+  }
 
   // Set OpenCL Version.
   Opts.OpenCL = Std.isOpenCL();
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index e118dda4780e2..81c836fe60452 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -159,7 +159,8 @@ void HLSLExternalSemaSource::defineHLSLMatrixAlias() {
                                                   SourceLocation(), ColsParam));
   TemplateParams.emplace_back(ColsParam);
 
-  const unsigned MaxMatDim = 4;
+  const unsigned MaxMatDim = SemaPtr->getLangOpts().MaxMatrixDimension;
+  ;
   auto *MaxRow = IntegerLiteral::Create(
       AST, llvm::APInt(AST.getIntWidth(AST.IntTy), MaxMatDim), AST.IntTy,
       SourceLocation());
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 063db05665af1..de37ee8ba5783 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -16240,9 +16240,9 @@ getAndVerifyMatrixDimension(Expr *Expr, StringRef Name, Sema &S) {
     return {};
   }
   uint64_t Dim = Value->getZExtValue();
-  if (!ConstantMatrixType::isDimensionValid(Dim)) {
+  if (Dim == 0 || Dim > S.Context.getLangOpts().MaxMatrixDimension) {
     S.Diag(Expr->getBeginLoc(), diag::err_builtin_matrix_invalid_dimension)
-        << Name << ConstantMatrixType::getMaxElementsPerDimension();
+        << Name << S.Context.getLangOpts().MaxMatrixDimension;
     return {};
   }
   return Dim;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index a9e7c34de94f4..811295c566a66 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -2517,12 +2517,12 @@ QualType Sema::BuildMatrixType(QualType ElementTy, Expr *NumRows, Expr *NumCols,
     Diag(AttrLoc, diag::err_attribute_zero_size) << "matrix" << ColRange;
     return QualType();
   }
-  if (!ConstantMatrixType::isDimensionValid(MatrixRows)) {
+  if (MatrixRows > Context.getLangOpts().MaxMatrixDimension) {
     Diag(AttrLoc, diag::err_attribute_size_too_large)
         << RowRange << "matrix row";
     return QualType();
   }
-  if (!ConstantMatrixType::isDimensionValid(MatrixColumns)) {
+  if (MatrixColumns > Context.getLangOpts().MaxMatrixDimension) {
     Diag(AttrLoc, diag::err_attribute_size_too_large)
         << ColRange << "matrix column";
     return QualType();

[llvm-beanz]

This change isn't NFC. You're not just moving a set value out of a header, you're also changing how it applies to the underlying matrix_type attribute. That warrants tests.

[farzonl]

This change isn't NFC. You're not just moving a set value out of a header, you're also changing how it applies to the underlying matrix_type attribute. That warrants tests.

We already have the test that confirm behavior differences that confirm the impact on the underlying matrix_type but there are no explicit tests on
Language option values. From looking how others have done this via
git grep -n "Invocation.getLangOpts()" -- clang/unittests/
It is all just looking at the getter() value. No one is changing a LangOpts and check if it impacts sema or ast generation.

There are tests like that in ASTImporterTest.cpp

clang/unittests/AST/ASTImporterTest.cpp:670:  testImport("typedef int __attribute__((matrix_type(5, 5))) declToImport;",
clang/unittests/AST/ASTImporterTest.cpp:681:        using declToImport = T __attribute__((matrix_type(Rows, Cols)));

but all of those aren't testing impact on matrix_type but rather its visibility with use of the -fenable-matrix flag.

If you are looking for sema tests maybe this should be a Options.td and not a LangOpt? then it would be easier to change the value per runline and show changing matrix dimmension constraints.

Anyways to recap these are the tests that exist:
clang/test/Sema/matrix-type-builtins.c
clang/test/SemaCXX/matrix-type-builtins.cpp
On the __builtin_matrix_column_major_load clang builtin

llvm-project/clang/test/Sema/matrix-type-builtins.c

Lines 67 to 70 in b358af1

	sx5x10_t a10 = __builtin_matrix_column_major_load(p1, 1ull << 21, 10, 6);
	// expected-error@-1 {{row dimension is outside the allowed range [1, 1048575}}
	sx5x10_t a11 = __builtin_matrix_column_major_load(p1, 10, 1ull << 21, 10);
	// expected-error@-1 {{column dimension is outside the allowed range [1, 1048575}}

llvm-project/clang/test/SemaCXX/matrix-type-builtins.cpp

Lines 73 to 77 in b358af1

	(void)__builtin_matrix_column_major_load(Ptr, constexpr_neg1(), 2, 4);
	// expected-error@-1 {{row dimension is outside the allowed range [1, 1048575]}}
	(void)__builtin_matrix_column_major_load(Ptr, 2, constexpr_neg1(), 4);
	// expected-error@-1 {{column dimension is outside the allowed range [1, 1048575]}}
	}

And in
clang/test/SemaHLSL/BuiltIns/matrix-basic_types-errors.hlsl

We are confirming that the constraint is applied on matrix<int, 5, 5> mat3;

llvm-project/clang/test/SemaHLSL/BuiltIns/matrix-basic_types-errors.hlsl

Lines 10 to 12 in b358af1

	matrix<int, 5, 5> mat3;
	// expected-error@-1 {{constraints not satisfied for alias template 'matrix' [with element = int, rows_count = 5, cols_count = 5]}}
	// expected-note@* {{because '5 <= 4' (5 <= 4) evaluated to false}}

Since the behavior differences are tested. I’ve just added an explicit compiler invocation test to confirm this number changes
When we change the language mode to HLSL. I think that’s all you wanted?

[llvm-beanz]

The behavior change is that in HLSL-mode the matrix_type attribute has reduced size limits.

We are confirming that the constraint is applied on matrix<int, 5, 5> mat3;

llvm-project/clang/test/SemaHLSL/BuiltIns/matrix-basic_types-errors.hlsl

Lines 10 to 12 in b358af1

	matrix<int, 5, 5> mat3;
	// expected-error@-1 {{constraints not satisfied for alias template 'matrix' [with element = int, rows_count = 5, cols_count = 5]}}
	// expected-note@* {{because '5 <= 4' (5 <= 4) evaluated to false}}

This is enforced by the matrix template, you're changing the implementation to also enforce this on the matrix_type attribute.

Since the behavior differences are tested. I’ve just added an explicit compiler invocation test to confirm this number changes When we change the language mode to HLSL. I think that’s all you wanted?

With this PR, clang should generate an error on this code:

using float8x8 = __attribute__((matrix_type(8,8))) float;

Which would not have been generated without this PR (See Compiler Explorer)

[llvm-beanz]

This looks reasonable to me. I've suggested a few extra test cases to consider. It looks like the C++ matrix dimensions are reasonably well covered over in https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/matrix-type.cpp.

[llvm-beanz]

A few other cases to consider:

using float8x8 = __attribute__((matrix_type(8,8))) float; // both dimensions too large

// these are likely tested in C/C++, but might be worth verifying or just adding them:
using floatNeg1x4 = __attribute__((matrix_type(-1,4))) float; // negative rows
using float4xNeg1 = __attribute__((matrix_type(4,-1))) float; // negative columns
using floatNeg1xNeg1 = __attribute__((matrix_type(-1,-1))) float; // both dimensions negative!

using float0x4 = __attribute__((matrix_type(0,4))) float; // zero rows?
using float4x0 = __attribute__((matrix_type(4,0))) float; // zero columns?
using float0x0 = __attribute__((matrix_type(0,0))) float; // both dimensions zero

[farzonl]

The current implementation does not do multiple errors so if both row/col are larger than 4 then we will just error matrix row size too large. The negative cases will just be treated as an unsinged integer so 4294967295 and will also be a row dimensions too large case. All of these will error.

[farzonl]

I can change the BuildMatrixType implementation to allow for both row and col errors.

[llvm-beanz]

Looks like a spurious ;.

Suggested change

	const unsigned MaxMatDim = SemaPtr->getLangOpts().MaxMatrixDimension;
	;
	const unsigned MaxMatDim = SemaPtr->getLangOpts().MaxMatrixDimension;

[farzonl]

clang-format didn't like deleting that line. so just removed the ;.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[hekota]

LGTM!