[Matrix] Add a row\col major toggle in the clang driver #167628

farzonl · 2025-11-12T03:20:23Z

define the new options in Options.td limit the naming to row-major or column-major.
In ToolChains/Clang.cpp limit the opt usage to only when -fenable-matrix is used.
make sure we set the flags llvm needs for the lower-matrix-intrinsics pass.

fixes llvm#167621 - define the new options in `Options.td` limit the naming to row-major or column-major. - In `ToolChains/Clang.cpp` limit the opt usage to only when `-fenable-matrix` is used. - make sure we set the flags llvm needs for the lower-matrix-intrinsics pass.

llvmbot · 2025-11-12T03:20:55Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Farzon Lotfi (farzonl)

Changes

fixes #167621

define the new options in Options.td limit the naming to row-major or column-major.
In ToolChains/Clang.cpp limit the opt usage to only when -fenable-matrix is used.
make sure we set the flags llvm needs for the lower-matrix-intrinsics pass.

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

3 Files Affected:

(modified) clang/include/clang/Options/Options.td (+5)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+11)
(added) clang/test/Driver/fmatrix-default-layout.c (+38)

diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td
index 2f7434d8afe11..e3f50c3187086 100644
--- a/clang/include/clang/Options/Options.td
+++ b/clang/include/clang/Options/Options.td
@@ -4642,6 +4642,11 @@ def fenable_matrix : Flag<["-"], "fenable-matrix">, Group<f_Group>,
     HelpText<"Enable matrix data type and related builtin functions">,
     MarshallingInfoFlag<LangOpts<"MatrixTypes">, hlsl.KeyPath>;
 
+def fmatrix_default_layout_EQ : Joined<["-"], "fmatrix-default-layout=">,
+    HelpText<"Set default matrix layout (row-major or column-major)">,
+    Values<"row-major,column-major">,
+    Group<f_Group>;
+
 defm raw_string_literals : BoolFOption<"raw-string-literals",
     LangOpts<"RawStringLiterals">, Default<std#".hasRawStringLiterals()">,
     PosFlag<SetTrue, [], [], "Enable">,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 80389937ee218..bcb97e8dfd897 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5692,6 +5692,17 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
     CmdArgs.push_back("-fenable-matrix");
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-enable-matrix");
+    // Only handle default layout if matrix is enabled
+    if (const Arg *A =
+            Args.getLastArg(options::OPT_fmatrix_default_layout_EQ)) {
+      StringRef Val = A->getValue();
+      if (Val == "row-major" || Val == "column-major") {
+        CmdArgs.push_back("-mllvm");
+        CmdArgs.push_back(Args.MakeArgString("-matrix-default-layout=" + Val));
+      } else {
+        D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
+      }
+    }
   }
 
   CodeGenOptions::FramePointerKind FPKeepKind =
diff --git a/clang/test/Driver/fmatrix-default-layout.c b/clang/test/Driver/fmatrix-default-layout.c
new file mode 100644
index 0000000000000..c89396f4452f6
--- /dev/null
+++ b/clang/test/Driver/fmatrix-default-layout.c
@@ -0,0 +1,38 @@
+// RUN: %clang --target=x86_64-linux-gnu -fenable-matrix -fmatrix-default-layout=column-major %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-COL-MAJOR
+// CHECK-COL-MAJOR:  -fenable-matrix
+// CHECK-COL-MAJOR:  -mllvm
+// CHECK-COL-MAJOR:  -enable-matrix
+// CHECK-COL-MAJOR:  -mllvm
+// CHECK-COL-MAJOR:  -matrix-default-layout=column-major
+
+// RUN: %clang --target=x86_64-linux-gnu -fenable-matrix -fmatrix-default-layout=row-major %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ROW-MAJOR
+// CHECK-ROW-MAJOR:  -fenable-matrix
+// CHECK-ROW-MAJOR:  -mllvm
+// CHECK-ROW-MAJOR:  -enable-matrix
+// CHECK-ROW-MAJOR:  -mllvm
+// CHECK-ROW-MAJOR:  -matrix-default-layout=row-major
+
+// RUN: not %clang --target=x86_64-linux-gnu -fenable-matrix -fmatrix-default-layout=error-major %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR-MAJOR
+// CHECK-ERROR-MAJOR: error: invalid value 'error-major' in '-fmatrix-default-layout=error-major'
+
+// RUN: %clang --target=x86_64-linux-gnu -fmatrix-default-layout=column-major %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-COL-MAJOR-DISABLED
+// CHECK-COL-MAJOR-DISABLED-NOT:  -fenable-matrix
+// CHECK-COL-MAJOR-DISABLED-NOT:  -mllvm
+// CHECK-COL-MAJOR-DISABLED-NOT:  -enable-matrix
+// CHECK-COL-MAJOR-DISABLED-NOT:  -mllvm
+// CHECK-COL-MAJOR-DISABLED-NOT:  -matrix-default-layout=column-major
+
+// RUN: %clang --target=x86_64-linux-gnu -fmatrix-default-layout=row-major %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ROW-MAJOR-DISABLED
+// CHECK-ROW-MAJOR-DISABLED-NOT:  -fenable-matrix
+// CHECK-ROW-MAJOR-DISABLED-NOT:  -mllvm
+// CHECK-ROW-MAJOR-DISABLED-NOT:  -enable-matrix
+// CHECK-ROW-MAJOR-DISABLED-NOT:  -mllvm
+// CHECK-ROW-MAJOR-DISABLED-NOT:  -matrix-default-layout=row-major
+
+// RUN: %clang --target=x86_64-linux-gnu -fenable-matrix  %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MATRIX-ENABLED
+// CHECK-MATRIX-ENABLED:  -fenable-matrix
+// CHECK-MATRIX-ENABLED:  -mllvm
+// CHECK-MATRIX-ENABLED:  -enable-matrix
+// CHECK-MATRIX-ENABLED-NOT:  -mllvm
+// CHECK-MATRIX-ENABLED-NOT:  -matrix-default-layout=row-major
+// CHECK-MATRIX-ENABLED-NOT:  -matrix-default-layout=column-major

farzonl · 2025-11-12T03:21:48Z

clang/lib/Driver/ToolChains/Clang.cpp

+            Args.getLastArg(options::OPT_fmatrix_default_layout_EQ)) {
+      StringRef Val = A->getValue();
+      if (Val == "row-major" || Val == "column-major") {
+        CmdArgs.push_back("-mllvm");


OPT_fenable_matrix did CmdArgs.push_back("-fenable-matrix"); I didn't understand why that was needed so didn't do the same thing for -matrix-default-layout=row-major or -matrix-default-layout= column-major.

farzonl · 2025-11-12T03:24:00Z

clang/test/Driver/fmatrix-default-layout.c

+// RUN: %clang --target=x86_64-linux-gnu -fenable-matrix  %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MATRIX-ENABLED
+// CHECK-MATRIX-ENABLED:  -fenable-matrix
+// CHECK-MATRIX-ENABLED:  -mllvm
+// CHECK-MATRIX-ENABLED:  -enable-matrix
+// CHECK-MATRIX-ENABLED-NOT:  -mllvm
+// CHECK-MATRIX-ENABLED-NOT:  -matrix-default-layout=row-major
+// CHECK-MATRIX-ENABLED-NOT:  -matrix-default-layout=column-major


I was thinking we would just have defaults stored in a LangOpt? So there woundn't be any need to have this flag if it wasn't specified.

if thats not right then I can change the default to be -mllvm -matrix-default-layout=column?

fhahn

OVerall seems reasonable to me, but in general I think overall it would be better to somehow explicitly mark the layout of a vector containing a matrix. Ideally we would only have to mark the producers of matrix values (mostly loads?), perhaps using an intrinsic (e.g. llvm.matrix.layout.row.major)?

Adding plumbing here seems fine as intermediate stop-gap solution, but would be good to follow up on handling this more generally.

It's probably worth mentioning the new flag herehttps://clang.llvm.org/docs/LanguageExtensions.html#matrix-types, as well as adding a release note.

cofibrant · 2025-11-12T11:22:08Z

Overall seems reasonable to me, but in general I think overall it would be better to somehow explicitly mark the layout of a vector containing a matrix. Ideally we would only have to mark the producers of matrix values (mostly loads?), perhaps using an intrinsic (e.g. llvm.matrix.layout.row.major)?

It seems maybe premature to enable these options before we have llvm.matrix.row.major.load and llvm.matrix.row.major.store intrinsics. Matrix loads and stores are currently always explicitly column major. (After adding these, the clang front-end will also have to be taught to use them when -matrix-default-layout=row-major.)

farzonl · 2025-11-12T17:59:51Z

Overall seems reasonable to me, but in general I think overall it would be better to somehow explicitly mark the layout of a vector containing a matrix. Ideally we would only have to mark the producers of matrix values (mostly loads?), perhaps using an intrinsic (e.g. llvm.matrix.layout.row.major)?

It seems maybe premature to enable these options before we have llvm.matrix.row.major.load and llvm.matrix.row.major.store intrinsics. Matrix loads and stores are currently always explicitly column major. (After adding these, the clang front-end will also have to be taught to use them when -matrix-default-layout=row-major.)

You aren't wrong. however there are things I can test today with HLSL that can benefit from row\colimn major toggles in the frontend. Specifically I need a clang cc1 flag that will be enables when we do -Zpr or -Zpc in HLSL. I need these flags to change the constructor layout behavior. Somethng I am experimenting with here: 0dbc5db. Finally I need to change the MatrixSubscriptExpr indexing behavior of vectors.

TLDR This change is a precursor PR for:

farzonl · 2025-11-12T18:19:17Z

@cofibrant @fhahn Since we are on the subject though are llvm.matrix.row.major.load and llvm.matrix.row.major.store intrinsics on your road map for implementation or is it something we should plan for the HLSL team to consider.

cofibrant · 2025-11-13T12:15:52Z

I've thought about this some more and I'll summarise my understanding. AFAIU, there are two separate concerns here:

The layout of matrix types in memory;
The layout used by LLVM when lowering matrix intrinsics.

As far as the matrix intrinsics are concerned, at least as they are exposed in the language reference, 2. is an implementation detail and is orthogonal from 1.

As an example, say I have a matrix A in memory in a column major layout and I call @llvm.matrix.column.major.load on it. With -matrix-default-layout=row-major, the lowering pass will generate sequence of strided loads fetching the rows of A. All subsequent operations will be row major until I call @llvm.matrix.column.major.store, at which point the a series of strided stores will be emitted, writing back the rows of whatever matrix I'm looking at now in a column major format.

To support row major memory layouts, we need to implement @llvm.matrix.row.major.* intrinsics and instruct the clang frontend to use these. In situations where all matrix operations are acting on row major matrices, there is then a performance reason to switch the default matrix layout used by the lowering pass to row major.

cofibrant · 2025-11-13T12:31:15Z

As for the roadmap, I'll defer to @fhahn, though it's certainly something I could look at if needed.

farzonl · 2025-11-13T13:34:12Z

@cofibrant what if i just change this line

- if (Val == "row-major" || Val == "column-major")
+ // TODO add “row-major” when row major load and store intrinsics are implemented 
+ if (Val == "column-major")

This should let us move forward with the flag and I can use it in memory layout.

cofibrant · 2025-11-13T14:36:18Z

What I'm trying to say is: it's fine to have this flag, but AFAIU the LLVM flag your patch sets doesn't change the memory layout of matrices, it changes whether LLVM uses vector registers to represent the rows or the columns of matrices by default. Changing the default memory layout is an orthogonal issue and will need additional intrinsics to support.

cofibrant · 2025-11-13T14:42:24Z

clang/lib/Driver/ToolChains/Clang.cpp

+        CmdArgs.push_back("-mllvm");
+        CmdArgs.push_back(Args.MakeArgString("-matrix-default-layout=" + Val));


So, if my understanding is correct, I think we don't want these lines. Moreover, I think the flag should really be called -fmatrix-memory-layout (or something similar) to distinguish it from the LLVM flag -matrix-default-layout which isn't talking about memory layout.

And then, yes, we shouldn't have an option for row-major until we've implemented the intrinsics.

Ok so you want no association of the clang driver flag and the llvm flag? Drop all the -mllvm -matrix-default-layout=* CmdArgs additions?

I'm hoping Florian chimes in but, yes, I think these flags are orthogonal

I think long term this could get confusing because it will create the posibility for someone to set -fmatrix-memory-layout=row-major and then do -mllvm -matrix-default-layout=column-major. And what I was trying to solve here was to have one place in the driver to keep all these options in sync.

farzonl · 2025-11-13T14:44:08Z

the LLVM flag your patch sets doesn't change the memory layout of matrices,

Yes but like I tried to explain earlier I want this flag to impact how we set memory layouts aswell. This is what one of my next patches will loook like:

diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index cc6ddf568d346..ceaba8976e50f 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -1913,10 +1913,20 @@ void InitListChecker::CheckMatrixType(const InitializedEntity &Entity,
 
   while (Index < IList->getNumInits()) {
     // Not a sublist: just consume directly.
-    unsigned ColMajorIndex = (Index % MT->getNumRows()) * MT->getNumColumns() +
-                             (Index / MT->getNumRows());
-    ElemEnt.setElementIndex(ColMajorIndex);
-    CheckSubElementType(ElemEnt, IList, ElemTy, ColMajorIndex, StructuredList,
+    unsigned ElementIndex;
+    
+    if (SemaRef.getLangOpts().MatrixRowMajor) {
+      // Row-major layout: use the index directly
+      ElementIndex = Index;
+    } else {
+      // Column-major layout: calculate the index
+      assert(SemaRef.getLangOpts().MatrixColMajor && "If Row Major is off Column Major must be on");
+      ElementIndex = (Index % MT->getNumRows()) * MT->getNumColumns() +
+                     (Index / MT->getNumRows());
+    }
+    
+    ElemEnt.setElementIndex(ElementIndex);
+    CheckSubElementType(ElemEnt, IList, ElemTy, ElementIndex, StructuredList,
                         StructuredIndex);
     ++Index;
   }

The idea being that we consume the initalizer list differently based on row\col major ordering.

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 12, 2025

farzonl commented Nov 12, 2025

View reviewed changes

farzonl requested a review from fhahn November 12, 2025 03:25

fhahn requested review from anemet, cofibrant and francisvm November 12, 2025 09:51

fhahn reviewed Nov 12, 2025

View reviewed changes

add new documentation to address pr concerns

e2f0a2d

cofibrant reviewed Nov 13, 2025

View reviewed changes

		CmdArgs.push_back("-mllvm");
		CmdArgs.push_back(Args.MakeArgString("-matrix-default-layout=" + Val));

[Matrix] Add a row\col major toggle in the clang driver #167628

Are you sure you want to change the base?

[Matrix] Add a row\col major toggle in the clang driver #167628

Conversation

farzonl commented Nov 12, 2025

Uh oh!

llvmbot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

farzonl Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farzonl Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

cofibrant commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

farzonl commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

farzonl commented Nov 12, 2025

Uh oh!

cofibrant commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cofibrant commented Nov 13, 2025

Uh oh!

farzonl commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cofibrant commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cofibrant Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cofibrant Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farzonl Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

cofibrant Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

farzonl Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farzonl commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Nov 12, 2025 •

edited

Loading

farzonl Nov 12, 2025 •

edited

Loading

cofibrant commented Nov 12, 2025 •

edited

Loading

farzonl commented Nov 12, 2025 •

edited

Loading

cofibrant commented Nov 13, 2025 •

edited

Loading

farzonl commented Nov 13, 2025 •

edited

Loading

cofibrant commented Nov 13, 2025 •

edited

Loading

cofibrant Nov 13, 2025 •

edited

Loading

cofibrant Nov 13, 2025 •

edited

Loading

farzonl Nov 13, 2025 •

edited

Loading

farzonl commented Nov 13, 2025 •

edited

Loading