[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG934bcaf10b57: [Matrix] Add __builtin_matrix_column_load to 
Clang. (authored by fhahn).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
  clang/test/SemaCXX/matrix-type-builtins.cpp

Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -39,3 +39,65 @@
   Mat3.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
+
+constexpr int constexpr1() { return 1; }
+constexpr int constexpr_neg1() { return -1; }
+
+void test_column_major_load_constexpr(unsigned *Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, 2, 2, constexpr1());
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+  (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]}}
+}
+
+struct IntWrapper {
+  operator int() {
+return 1;
+  }
+};
+
+void test_column_major_load_wrapper(unsigned *Ptr, IntWrapper ) {
+  (void)__builtin_matrix_column_major_load(Ptr, W, 2, 2);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(Ptr, 2, W, 2);
+  // expected-error@-1 {{column argument must be a constant unsigned integer expression}}
+}
+
+template 
+void test_column_major_load_temp(T Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, R, C, S);
+}
+
+void call_column_major_load_temp(unsigned *Ptr, unsigned X) {
+  (void)__builtin_matrix_column_major_load(Ptr, X, X, X);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  // expected-error@-2 {{column argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(X, 2, 2, 2);
+  // expected-error@-1 {{first argument must be a pointer to a valid matrix element type}}
+}
Index: clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -pedantic -std=c++11 -verify -triple=x86_64-apple-darwin9
+
+// Make sure we fail without -fenable-matrix when
+// __builtin_matrix_column_major_load is used to construct a new matrix type.
+void 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 271613.
fhahn added a comment.

Rebased after recent parent patches landed. Will commit shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
  clang/test/SemaCXX/matrix-type-builtins.cpp

Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -39,3 +39,65 @@
   Mat3.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
+
+constexpr int constexpr1() { return 1; }
+constexpr int constexpr_neg1() { return -1; }
+
+void test_column_major_load_constexpr(unsigned *Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, 2, 2, constexpr1());
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+  (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]}}
+}
+
+struct IntWrapper {
+  operator int() {
+return 1;
+  }
+};
+
+void test_column_major_load_wrapper(unsigned *Ptr, IntWrapper ) {
+  (void)__builtin_matrix_column_major_load(Ptr, W, 2, 2);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(Ptr, 2, W, 2);
+  // expected-error@-1 {{column argument must be a constant unsigned integer expression}}
+}
+
+template 
+void test_column_major_load_temp(T Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, R, C, S);
+}
+
+void call_column_major_load_temp(unsigned *Ptr, unsigned X) {
+  (void)__builtin_matrix_column_major_load(Ptr, X, X, X);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  // expected-error@-2 {{column argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(X, 2, 2, 2);
+  // expected-error@-1 {{first argument must be a pointer to a valid matrix element type}}
+}
Index: clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -pedantic -std=c++11 -verify -triple=x86_64-apple-darwin9
+
+// Make sure we fail without -fenable-matrix when
+// __builtin_matrix_column_major_load is used to construct a new matrix type.
+void column_major_load_with_stride(int *Ptr) {
+  

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D72781#2084077 , @rjmccall wrote:

> LGTM.


Thank you very much again John! This patch is pending on a few smallish 
improvements to the load/store intrinsics (D81472 
) and I'll land once that one is wrapped up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/include/clang/Sema/Sema.h:12126
+  ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+  ExprResult CallResult);
 

fhahn wrote:
> rjmccall wrote:
> > I don't think the word "overload" is doing anything in either of these 
> > method names.
> Removed Overload here and for `SemaBuiltinMatrixTranspose`
Thanks.



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

fhahn wrote:
> rjmccall wrote:
> > fhahn wrote:
> > > rjmccall wrote:
> > > > fhahn wrote:
> > > > > rjmccall wrote:
> > > > > > It's almost never correct to do "local" qualifier manipulation in 
> > > > > > Sema.  You want to remove the `const` qualifier, which means 
> > > > > > removing it through however much type sugar might be wrapping it.
> > > > > > 
> > > > > > In reality, though, you actually want to remove *all* qualifiers, 
> > > > > > not just `const`.  So you should just use `getUnqualifiedType()`.  
> > > > > > (And then you need to make sure that the IRGen code works on 
> > > > > > arbitrarily-qualified pointers.  It should already handle address 
> > > > > > spaces unless you're doing something very strange.  To handle 
> > > > > > `volatile`, you just need to be able to pass down a `volatile` flag 
> > > > > > to your helper function.  The other qualifiers should all either 
> > > > > > not require special handling or not be allowed on integer/float 
> > > > > > types.)
> > > > > > In reality, though, you actually want to remove *all* qualifiers, 
> > > > > > not just const. So you should just use getUnqualifiedType(). (And 
> > > > > > then you need to make sure that the IRGen code works on 
> > > > > > arbitrarily-qualified pointers. It should already handle address 
> > > > > > spaces unless you're doing something very strange. To handle 
> > > > > > volatile, you just need to be able to pass down a volatile flag to 
> > > > > > your helper function. The other qualifiers should all either not 
> > > > > > require special handling or not be allowed on integer/float types.)
> > > > > 
> > > > > Updated. Currently volatile cannot be specified for the 
> > > > > `@llvm.matrix.columnwise.load/store` builtins. I'll put up an update 
> > > > > for the intrinsics, for now I added an assertion in IRGen. I recently 
> > > > > put up a patch that allows adding nuw/nsw info to the multiply 
> > > > > builtin using operand bundles (D81166). For volatile, we could add 
> > > > > another bundle or a boolean argument like we have for memcpy 
> > > > > intrinsic I think. I am leaning towards an operand bundle version for 
> > > > > this optional argument. Do you have any preference?
> > > > The only thing I really care about is that it can't be dropped 
> > > > implicitly.  That's not legal with a bundle, but it's maybe a little 
> > > > more likely as an error of omission.  On the other hand, you do need to 
> > > > pass down an alignment, and that really shouldn't be an optional 
> > > > argument, so maybe that's an opportunity to add a `volatile` argument 
> > > > as well.
> > > I think for alignment we can use the align call attribute, which is what 
> > > I am using in the latest update.
> > Is there a reason this intrinsic can't be changed?  You don't need to do it 
> > in this patch, but using the "align" attribute as call-site attribute 
> > that's only meaningful on certain initrinsics seems like a really poor 
> > representation choice, especially for something as semantically important 
> > as an alignment assumption.
> I think we should be able to change them. I put up D81472 to update the 
> load/store intrinsics to update the name, types of stride/rows/columns and 
> add a `IsVolatile` flag. We could also pass the alignment as an extra 
> parameter, but it seems like the `align` attribute already provides a way to 
> specify alignment on a per-argument basis. Using it would mean we don't have 
> to teach various passes that use/propagate alignment info about the new 
> intrinsics.
Ah, I'd forgotten that when we updated memcpy etc., we made it specify an 
alignment with an alignment parameter attribute instead of a separate argument. 
 Yes, that's fine to imitate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 269556.
fhahn marked 2 inline comments as done.
fhahn added a comment.

Adjust naming as suggested, pass through volatile flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
  clang/test/SemaCXX/matrix-type-builtins.cpp

Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -39,3 +39,65 @@
   Mat3.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
+
+constexpr int constexpr1() { return 1; }
+constexpr int constexpr_neg1() { return -1; }
+
+void test_column_major_load_constexpr(unsigned *Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, 2, 2, constexpr1());
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+  (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]}}
+}
+
+struct IntWrapper {
+  operator int() {
+return 1;
+  }
+};
+
+void test_column_major_load_wrapper(unsigned *Ptr, IntWrapper ) {
+  (void)__builtin_matrix_column_major_load(Ptr, W, 2, 2);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(Ptr, 2, W, 2);
+  // expected-error@-1 {{column argument must be a constant unsigned integer expression}}
+}
+
+template 
+void test_column_major_load_temp(T Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, R, C, S);
+}
+
+void call_column_major_load_temp(unsigned *Ptr, unsigned X) {
+  (void)__builtin_matrix_column_major_load(Ptr, X, X, X);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  // expected-error@-2 {{column argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(X, 2, 2, 2);
+  // expected-error@-1 {{first argument must be a pointer to a valid matrix element type}}
+}
Index: clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -pedantic -std=c++11 -verify -triple=x86_64-apple-darwin9
+
+// Make sure we fail without -fenable-matrix when
+// __builtin_matrix_column_major_load is used to construct a new matrix type.
+void 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:12126
+  ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+  ExprResult CallResult);
 

rjmccall wrote:
> I don't think the word "overload" is doing anything in either of these method 
> names.
Removed Overload here and for `SemaBuiltinMatrixTranspose`



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

rjmccall wrote:
> fhahn wrote:
> > rjmccall wrote:
> > > fhahn wrote:
> > > > rjmccall wrote:
> > > > > It's almost never correct to do "local" qualifier manipulation in 
> > > > > Sema.  You want to remove the `const` qualifier, which means removing 
> > > > > it through however much type sugar might be wrapping it.
> > > > > 
> > > > > In reality, though, you actually want to remove *all* qualifiers, not 
> > > > > just `const`.  So you should just use `getUnqualifiedType()`.  (And 
> > > > > then you need to make sure that the IRGen code works on 
> > > > > arbitrarily-qualified pointers.  It should already handle address 
> > > > > spaces unless you're doing something very strange.  To handle 
> > > > > `volatile`, you just need to be able to pass down a `volatile` flag 
> > > > > to your helper function.  The other qualifiers should all either not 
> > > > > require special handling or not be allowed on integer/float types.)
> > > > > In reality, though, you actually want to remove *all* qualifiers, not 
> > > > > just const. So you should just use getUnqualifiedType(). (And then 
> > > > > you need to make sure that the IRGen code works on 
> > > > > arbitrarily-qualified pointers. It should already handle address 
> > > > > spaces unless you're doing something very strange. To handle 
> > > > > volatile, you just need to be able to pass down a volatile flag to 
> > > > > your helper function. The other qualifiers should all either not 
> > > > > require special handling or not be allowed on integer/float types.)
> > > > 
> > > > Updated. Currently volatile cannot be specified for the 
> > > > `@llvm.matrix.columnwise.load/store` builtins. I'll put up an update 
> > > > for the intrinsics, for now I added an assertion in IRGen. I recently 
> > > > put up a patch that allows adding nuw/nsw info to the multiply builtin 
> > > > using operand bundles (D81166). For volatile, we could add another 
> > > > bundle or a boolean argument like we have for memcpy intrinsic I think. 
> > > > I am leaning towards an operand bundle version for this optional 
> > > > argument. Do you have any preference?
> > > The only thing I really care about is that it can't be dropped 
> > > implicitly.  That's not legal with a bundle, but it's maybe a little more 
> > > likely as an error of omission.  On the other hand, you do need to pass 
> > > down an alignment, and that really shouldn't be an optional argument, so 
> > > maybe that's an opportunity to add a `volatile` argument as well.
> > I think for alignment we can use the align call attribute, which is what I 
> > am using in the latest update.
> Is there a reason this intrinsic can't be changed?  You don't need to do it 
> in this patch, but using the "align" attribute as call-site attribute that's 
> only meaningful on certain initrinsics seems like a really poor 
> representation choice, especially for something as semantically important as 
> an alignment assumption.
I think we should be able to change them. I put up D81472 to update the 
load/store intrinsics to update the name, types of stride/rows/columns and add 
a `IsVolatile` flag. We could also pass the alignment as an extra parameter, 
but it seems like the `align` attribute already provides a way to specify 
alignment on a per-argument basis. Using it would mean we don't have to teach 
various passes that use/propagate alignment info about the new intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:4708
+  /// conversion.
+  ExprResult tryConvertExprToTy(Expr *E, QualType Ty);
+

Please spell out "type" in the method name.



Comment at: clang/include/clang/Sema/Sema.h:12126
+  ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+  ExprResult CallResult);
 

I don't think the word "overload" is doing anything in either of these method 
names.



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

fhahn wrote:
> rjmccall wrote:
> > fhahn wrote:
> > > rjmccall wrote:
> > > > It's almost never correct to do "local" qualifier manipulation in Sema. 
> > > >  You want to remove the `const` qualifier, which means removing it 
> > > > through however much type sugar might be wrapping it.
> > > > 
> > > > In reality, though, you actually want to remove *all* qualifiers, not 
> > > > just `const`.  So you should just use `getUnqualifiedType()`.  (And 
> > > > then you need to make sure that the IRGen code works on 
> > > > arbitrarily-qualified pointers.  It should already handle address 
> > > > spaces unless you're doing something very strange.  To handle 
> > > > `volatile`, you just need to be able to pass down a `volatile` flag to 
> > > > your helper function.  The other qualifiers should all either not 
> > > > require special handling or not be allowed on integer/float types.)
> > > > In reality, though, you actually want to remove *all* qualifiers, not 
> > > > just const. So you should just use getUnqualifiedType(). (And then you 
> > > > need to make sure that the IRGen code works on arbitrarily-qualified 
> > > > pointers. It should already handle address spaces unless you're doing 
> > > > something very strange. To handle volatile, you just need to be able to 
> > > > pass down a volatile flag to your helper function. The other qualifiers 
> > > > should all either not require special handling or not be allowed on 
> > > > integer/float types.)
> > > 
> > > Updated. Currently volatile cannot be specified for the 
> > > `@llvm.matrix.columnwise.load/store` builtins. I'll put up an update for 
> > > the intrinsics, for now I added an assertion in IRGen. I recently put up 
> > > a patch that allows adding nuw/nsw info to the multiply builtin using 
> > > operand bundles (D81166). For volatile, we could add another bundle or a 
> > > boolean argument like we have for memcpy intrinsic I think. I am leaning 
> > > towards an operand bundle version for this optional argument. Do you have 
> > > any preference?
> > The only thing I really care about is that it can't be dropped implicitly.  
> > That's not legal with a bundle, but it's maybe a little more likely as an 
> > error of omission.  On the other hand, you do need to pass down an 
> > alignment, and that really shouldn't be an optional argument, so maybe 
> > that's an opportunity to add a `volatile` argument as well.
> I think for alignment we can use the align call attribute, which is what I am 
> using in the latest update.
Is there a reason this intrinsic can't be changed?  You don't need to do it in 
this patch, but using the "align" attribute as call-site attribute that's only 
meaningful on certain initrinsics seems like a really poor representation 
choice, especially for something as semantically important as an alignment 
assumption.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 9 inline comments as done.
fhahn added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10789
+def err_builtin_matrix_scalar_constant_unsigned_expr_arg: Error<
+  "%0 argument must be a constant unsigned integer expression">;
+

rjmccall wrote:
> These are the same now.
Ah yes, it has been unused actually. Dropped.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2403
+Address Src = Address::invalid();
+Src = EmitPointerWithAlignment(E->getArg(0));
+

rjmccall wrote:
> This can be simplified now.
Folded the 2 statements.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2409
+Src.getPointer(), ResultTy->getNumRows(), ResultTy->getNumColumns(),
+Stride, "matrix");
+return RValue::get(Result);

rjmccall wrote:
> You should honor the alignment of `Src`.  If you emit a bunch of scattered 
> loads, e.g. if the stride is not a constant, it might just be a cap on the 
> alignment of the individual loads rather than a general optimization; but 
> still, you should honor it.
Pass the alignment through to the builder. It sets the align attribute for the 
pointer argument now



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

rjmccall wrote:
> fhahn wrote:
> > rjmccall wrote:
> > > It's almost never correct to do "local" qualifier manipulation in Sema.  
> > > You want to remove the `const` qualifier, which means removing it through 
> > > however much type sugar might be wrapping it.
> > > 
> > > In reality, though, you actually want to remove *all* qualifiers, not 
> > > just `const`.  So you should just use `getUnqualifiedType()`.  (And then 
> > > you need to make sure that the IRGen code works on arbitrarily-qualified 
> > > pointers.  It should already handle address spaces unless you're doing 
> > > something very strange.  To handle `volatile`, you just need to be able 
> > > to pass down a `volatile` flag to your helper function.  The other 
> > > qualifiers should all either not require special handling or not be 
> > > allowed on integer/float types.)
> > > In reality, though, you actually want to remove *all* qualifiers, not 
> > > just const. So you should just use getUnqualifiedType(). (And then you 
> > > need to make sure that the IRGen code works on arbitrarily-qualified 
> > > pointers. It should already handle address spaces unless you're doing 
> > > something very strange. To handle volatile, you just need to be able to 
> > > pass down a volatile flag to your helper function. The other qualifiers 
> > > should all either not require special handling or not be allowed on 
> > > integer/float types.)
> > 
> > Updated. Currently volatile cannot be specified for the 
> > `@llvm.matrix.columnwise.load/store` builtins. I'll put up an update for 
> > the intrinsics, for now I added an assertion in IRGen. I recently put up a 
> > patch that allows adding nuw/nsw info to the multiply builtin using operand 
> > bundles (D81166). For volatile, we could add another bundle or a boolean 
> > argument like we have for memcpy intrinsic I think. I am leaning towards an 
> > operand bundle version for this optional argument. Do you have any 
> > preference?
> The only thing I really care about is that it can't be dropped implicitly.  
> That's not legal with a bundle, but it's maybe a little more likely as an 
> error of omission.  On the other hand, you do need to pass down an alignment, 
> and that really shouldn't be an optional argument, so maybe that's an 
> opportunity to add a `volatile` argument as well.
I think for alignment we can use the align call attribute, which is what I am 
using in the latest update.



Comment at: clang/lib/Sema/SemaChecking.cpp:15094
+Sema::SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+   ExprResult CallResult) {
+  if (checkArgCount(*this, TheCall, 4))

rjmccall wrote:
> You do need to check whether your extension is enabled in this builtin.
Done and also added a test.



Comment at: clang/lib/Sema/SemaChecking.cpp:15112
+TheCall->setArg(0, PtrExpr);
+  }
+

rjmccall wrote:
> You can just bail out early here (set a dependent type on the expression and 
> return) if `PtrExpr` is type-dependent.
added early exit.



Comment at: clang/lib/Sema/SemaChecking.cpp:15185
+  uint64_t Stride = Value.getZExtValue();
+  if (MaybeRows && Stride < *MaybeRows) {
+Diag(StrideExpr->getBeginLoc(),

rjmccall wrote:
> Might as well hoist the `MaybeRows` check up so that we skip this whole thing 
> if we don't have a row count.
Moved to the outer if.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 269378.
fhahn marked 5 inline comments as done.
fhahn added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Simplified code as suggested, check if matrix type extensions is enabled (and 
add test) and set align attribute for pointer argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins-disabled.cpp
  clang/test/SemaCXX/matrix-type-builtins.cpp
  llvm/include/llvm/IR/MatrixBuilder.h

Index: llvm/include/llvm/IR/MatrixBuilder.h
===
--- llvm/include/llvm/IR/MatrixBuilder.h
+++ llvm/include/llvm/IR/MatrixBuilder.h
@@ -56,10 +56,9 @@
   /// \p Rows- Number of rows in matrix (must be a constant)
   /// \p Columns - Number of columns in matrix (must be a constant)
   /// \p Stride  - Space between columns
-  CallInst *CreateMatrixColumnwiseLoad(Value *DataPtr, unsigned Rows,
-   unsigned Columns, Value *Stride,
-   const Twine  = "") {
-
+  CallInst *CreateMatrixColumnwiseLoad(Value *DataPtr, unsigned Alignment,
+   unsigned Rows, unsigned Columns,
+   Value *Stride, const Twine  = "") {
 // Deal with the pointer
 PointerType *PtrTy = cast(DataPtr->getType());
 Type *EltTy = PtrTy->getElementType();
@@ -72,7 +71,11 @@
 Function *TheFn = Intrinsic::getDeclaration(
 getModule(), Intrinsic::matrix_columnwise_load, OverloadedTypes);
 
-return B.CreateCall(TheFn->getFunctionType(), TheFn, Ops, Name);
+CallInst *Call = B.CreateCall(TheFn->getFunctionType(), TheFn, Ops, Name);
+Attribute AlignAttr =
+Attribute::getWithAlignment(Call->getContext(), Align(Alignment));
+Call->addAttribute(1, AlignAttr);
+return Call;
   }
 
   /// Create a columnwise, strided matrix store.
Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -39,3 +39,65 @@
   Mat3.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
+
+constexpr int constexpr1() { return 1; }
+constexpr int constexpr_neg1() { return -1; }
+
+void test_column_major_load_constexpr(unsigned *Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, 2, 2, constexpr1());
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+  (void)__builtin_matrix_column_major_load(Ptr, constexpr_neg1(), 2, 4);
+  // expected-error@-1 {{row dimension is outside the 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

fhahn wrote:
> rjmccall wrote:
> > It's almost never correct to do "local" qualifier manipulation in Sema.  
> > You want to remove the `const` qualifier, which means removing it through 
> > however much type sugar might be wrapping it.
> > 
> > In reality, though, you actually want to remove *all* qualifiers, not just 
> > `const`.  So you should just use `getUnqualifiedType()`.  (And then you 
> > need to make sure that the IRGen code works on arbitrarily-qualified 
> > pointers.  It should already handle address spaces unless you're doing 
> > something very strange.  To handle `volatile`, you just need to be able to 
> > pass down a `volatile` flag to your helper function.  The other qualifiers 
> > should all either not require special handling or not be allowed on 
> > integer/float types.)
> > In reality, though, you actually want to remove *all* qualifiers, not just 
> > const. So you should just use getUnqualifiedType(). (And then you need to 
> > make sure that the IRGen code works on arbitrarily-qualified pointers. It 
> > should already handle address spaces unless you're doing something very 
> > strange. To handle volatile, you just need to be able to pass down a 
> > volatile flag to your helper function. The other qualifiers should all 
> > either not require special handling or not be allowed on integer/float 
> > types.)
> 
> Updated. Currently volatile cannot be specified for the 
> `@llvm.matrix.columnwise.load/store` builtins. I'll put up an update for the 
> intrinsics, for now I added an assertion in IRGen. I recently put up a patch 
> that allows adding nuw/nsw info to the multiply builtin using operand bundles 
> (D81166). For volatile, we could add another bundle or a boolean argument 
> like we have for memcpy intrinsic I think. I am leaning towards an operand 
> bundle version for this optional argument. Do you have any preference?
The only thing I really care about is that it can't be dropped implicitly.  
That's not legal with a bundle, but it's maybe a little more likely as an error 
of omission.  On the other hand, you do need to pass down an alignment, and 
that really shouldn't be an optional argument, so maybe that's an opportunity 
to add a `volatile` argument as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10789
+def err_builtin_matrix_scalar_constant_unsigned_expr_arg: Error<
+  "%0 argument must be a constant unsigned integer expression">;
+

These are the same now.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2403
+Address Src = Address::invalid();
+Src = EmitPointerWithAlignment(E->getArg(0));
+

This can be simplified now.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2409
+Src.getPointer(), ResultTy->getNumRows(), ResultTy->getNumColumns(),
+Stride, "matrix");
+return RValue::get(Result);

You should honor the alignment of `Src`.  If you emit a bunch of scattered 
loads, e.g. if the stride is not a constant, it might just be a cap on the 
alignment of the individual loads rather than a general optimization; but 
still, you should honor it.



Comment at: clang/lib/Sema/SemaChecking.cpp:15178
+Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+<< 2 << 1;
+ArgError = true;

fhahn wrote:
> rjmccall wrote:
> > It'd be nice to have comments for these magic values, like `/*stride*/ 2`.
> On second thought, the benefits of the magic numbers is rather small. I 
> updated the diagnostics to take strings with the names of the arguments 
> directly.
Actually, isn't this diagnostic redundant with the conversion you do in 
ApplyArgumentConversions?



Comment at: clang/lib/Sema/SemaChecking.cpp:15094
+Sema::SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall,
+   ExprResult CallResult) {
+  if (checkArgCount(*this, TheCall, 4))

You do need to check whether your extension is enabled in this builtin.



Comment at: clang/lib/Sema/SemaChecking.cpp:15112
+TheCall->setArg(0, PtrExpr);
+  }
+

You can just bail out early here (set a dependent type on the expression and 
return) if `PtrExpr` is type-dependent.



Comment at: clang/lib/Sema/SemaChecking.cpp:15185
+  uint64_t Stride = Value.getZExtValue();
+  if (MaybeRows && Stride < *MaybeRows) {
+Diag(StrideExpr->getBeginLoc(),

Might as well hoist the `MaybeRows` check up so that we skip this whole thing 
if we don't have a row count.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 12 inline comments as done.
fhahn added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:15107
+  {
+ExprResult PtrConv = DefaultLvalueConversion(PtrExpr);
+if (PtrConv.isInvalid())

rjmccall wrote:
> You should be doing `DefaultFunctionArrayLvalueConversion` here, which will 
> eliminate all the special cases for arrays, both below and in IRGen.
> 
> It would've been fine to do that for your other builtin, too, it just wasn't 
> necessary because it can never allow pointers.
Great thanks! That & together with `DependentTy` seems to solve the issue 
related to pointer type template expressions.



Comment at: clang/lib/Sema/SemaChecking.cpp:15110
+  return PtrConv;
+PtrExpr = PtrConv.get();
+  }

rjmccall wrote:
> Probably best to write it back into the call immediately at this point.
Updated to update the call immediately after conversions here and below.



Comment at: clang/lib/Sema/SemaChecking.cpp:15116
+  if (auto *SubstTy = PtrTy->getAs())
+PtrTy = SubstTy->getReplacementType();
+

rjmccall wrote:
> Thinking that you need to do this is a huge indicator that you're doing 
> something wrong later.  You should not have to remove  type sugar.
Not needed anymore, as mentioned above. Now the `remove_pointer` test also 
works :)



Comment at: clang/lib/Sema/SemaChecking.cpp:15121
+Diag(PtrExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg) << 0;
+ArgError = true;
+  } else {

rjmccall wrote:
> You need to allow this expression to be dependently-typed.  There's a generic 
> `DependentTy` that you can use as the result type of the call in this case.
I've updated the code to use `DependentTy` if any of the parts of the result 
matrix type is still dependently-typed.



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

rjmccall wrote:
> It's almost never correct to do "local" qualifier manipulation in Sema.  You 
> want to remove the `const` qualifier, which means removing it through however 
> much type sugar might be wrapping it.
> 
> In reality, though, you actually want to remove *all* qualifiers, not just 
> `const`.  So you should just use `getUnqualifiedType()`.  (And then you need 
> to make sure that the IRGen code works on arbitrarily-qualified pointers.  It 
> should already handle address spaces unless you're doing something very 
> strange.  To handle `volatile`, you just need to be able to pass down a 
> `volatile` flag to your helper function.  The other qualifiers should all 
> either not require special handling or not be allowed on integer/float types.)
> In reality, though, you actually want to remove *all* qualifiers, not just 
> const. So you should just use getUnqualifiedType(). (And then you need to 
> make sure that the IRGen code works on arbitrarily-qualified pointers. It 
> should already handle address spaces unless you're doing something very 
> strange. To handle volatile, you just need to be able to pass down a volatile 
> flag to your helper function. The other qualifiers should all either not 
> require special handling or not be allowed on integer/float types.)

Updated. Currently volatile cannot be specified for the 
`@llvm.matrix.columnwise.load/store` builtins. I'll put up an update for the 
intrinsics, for now I added an assertion in IRGen. I recently put up a patch 
that allows adding nuw/nsw info to the multiply builtin using operand bundles 
(D81166). For volatile, we could add another bundle or a boolean argument like 
we have for memcpy intrinsic I think. I am leaning towards an operand bundle 
version for this optional argument. Do you have any preference?



Comment at: clang/lib/Sema/SemaChecking.cpp:15138
+  if (RowsExpr->isValueDependent() || RowsExpr->isTypeDependent() ||
+  ColumnsExpr->isValueDependent() || ColumnsExpr->isTypeDependent()) {
+QualType ReturnType = Context.getDependentSizedMatrixType(

rjmccall wrote:
> Value dependence implies type dependence.  Butt you can't do these checks 
> until after you've at least lowered placeholders.
> 
> It's not really necessary to build a DependentSizedMatrixType here rather 
> than just using DependentTy.  It's not a bad thing to do — it *could* enable 
> better type-checking of templates, like if you did this load and then had 
> code trying to do a non-matrix operation on the result you could maybe reject 
> that immediately instead of waiting for instantiation — but it's not really 
> necessary, either.
> Value dependence implies type dependence. Butt you can't do these checks 
> until after you've at least lowered placeholders.

I moved the conversion earlier.

> It's not really necessary to build a DependentSizedMatrixType here rather 
> than just 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 269363.
fhahn added a comment.

Updated to

- use DefaultFunctionArrayLvalueConversion for pointer conversion, use 
getAs subsequently
- return Context.DependentTy if any part of the result matrix type is still 
type-dependent
- add assertion & todo for volatile
- pass string arguments to diagnostics instead of magic integers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins.cpp

Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -39,3 +39,65 @@
   Mat3.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
+
+constexpr int constexpr1() { return 1; }
+constexpr int constexpr_neg1() { return -1; }
+
+void test_column_major_load_constexpr(unsigned *Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, 2, 2, constexpr1());
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+  (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]}}
+}
+
+struct IntWrapper {
+  operator int() {
+return 1;
+  }
+};
+
+void test_column_major_load_wrapper(unsigned *Ptr, IntWrapper ) {
+  (void)__builtin_matrix_column_major_load(Ptr, W, 2, 2);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(Ptr, 2, W, 2);
+  // expected-error@-1 {{column argument must be a constant unsigned integer expression}}
+}
+
+template 
+void test_column_major_load_temp(T Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, R, C, S);
+}
+
+void call_column_major_load_temp(unsigned *Ptr, unsigned X) {
+  (void)__builtin_matrix_column_major_load(Ptr, X, X, X);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  // expected-error@-2 {{column argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(X, 2, 2, 2);
+  // expected-error@-1 {{first argument must be a pointer to a valid matrix element type}}
+}
Index: clang/test/Sema/matrix-type-builtins.c
===
--- clang/test/Sema/matrix-type-builtins.c
+++ clang/test/Sema/matrix-type-builtins.c
@@ -20,3 +20,49 @@
   ix3x3 m = __builtin_matrix_transpose(c);
   // expected-error@-1 {{initializing 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

`SubstTemplateTypeParmType` is a "sugar" type node, like a `typedef`, and code 
should generally be looking through it automatically by using `getAs` rather 
than `isa` / `dyn_cast`.




Comment at: clang/lib/Sema/SemaChecking.cpp:15107
+  {
+ExprResult PtrConv = DefaultLvalueConversion(PtrExpr);
+if (PtrConv.isInvalid())

You should be doing `DefaultFunctionArrayLvalueConversion` here, which will 
eliminate all the special cases for arrays, both below and in IRGen.

It would've been fine to do that for your other builtin, too, it just wasn't 
necessary because it can never allow pointers.



Comment at: clang/lib/Sema/SemaChecking.cpp:15110
+  return PtrConv;
+PtrExpr = PtrConv.get();
+  }

Probably best to write it back into the call immediately at this point.



Comment at: clang/lib/Sema/SemaChecking.cpp:15116
+  if (auto *SubstTy = PtrTy->getAs())
+PtrTy = SubstTy->getReplacementType();
+

Thinking that you need to do this is a huge indicator that you're doing 
something wrong later.  You should not have to remove  type sugar.



Comment at: clang/lib/Sema/SemaChecking.cpp:15121
+Diag(PtrExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg) << 0;
+ArgError = true;
+  } else {

You need to allow this expression to be dependently-typed.  There's a generic 
`DependentTy` that you can use as the result type of the call in this case.



Comment at: clang/lib/Sema/SemaChecking.cpp:15128
+else
+  llvm_unreachable("Pointer Expression must be a pointer or an array");
+

`getAs()` is the right way to do this.  (You won't need 
`getAsArrayType` if you decay arrays properly above.)



Comment at: clang/lib/Sema/SemaChecking.cpp:15130
+
+ElementTy.removeLocalConst();
+if (!ConstantMatrixType::isValidElementType(ElementTy)) {

It's almost never correct to do "local" qualifier manipulation in Sema.  You 
want to remove the `const` qualifier, which means removing it through however 
much type sugar might be wrapping it.

In reality, though, you actually want to remove *all* qualifiers, not just 
`const`.  So you should just use `getUnqualifiedType()`.  (And then you need to 
make sure that the IRGen code works on arbitrarily-qualified pointers.  It 
should already handle address spaces unless you're doing something very 
strange.  To handle `volatile`, you just need to be able to pass down a 
`volatile` flag to your helper function.  The other qualifiers should all 
either not require special handling or not be allowed on integer/float types.)



Comment at: clang/lib/Sema/SemaChecking.cpp:15138
+  if (RowsExpr->isValueDependent() || RowsExpr->isTypeDependent() ||
+  ColumnsExpr->isValueDependent() || ColumnsExpr->isTypeDependent()) {
+QualType ReturnType = Context.getDependentSizedMatrixType(

Value dependence implies type dependence.  Butt you can't do these checks until 
after you've at least lowered placeholders.

It's not really necessary to build a DependentSizedMatrixType here rather than 
just using DependentTy.  It's not a bad thing to do — it *could* enable better 
type-checking of templates, like if you did this load and then had code trying 
to do a non-matrix operation on the result you could maybe reject that 
immediately instead of waiting for instantiation — but it's not really 
necessary, either.



Comment at: clang/lib/Sema/SemaChecking.cpp:15178
+Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+<< 2 << 1;
+ArgError = true;

It'd be nice to have comments for these magic values, like `/*stride*/ 2`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 269249.
fhahn added a comment.

Ping.

Applied feedback from D72778  to this patch, 
improved tests, support conversions/placeholders.

One thing I am not sure is how to properly handle template substitutions for 
the pointer expression for code like the one below, where we need to apply 
substitutions to get the actual pointer type. Currently the patch looks through 
SubstTemplateTypeParmType types in Sema to construct the result type. Should we 
look through SubstTemplateTypeParmType in IRGen too to decide whether to call 
EmitPointerWithAlignment or EmitArrayToPointerDecay? Or is there a place in 
sema that should get rid of the substitution (perhaps in SemaChecking.cpp)?

  template  struct remove_pointer {
  typedef T type;
  };
  
  template  struct remove_pointer{
  typedef typename remove_pointer::type type;
  };
  
  // Same as column_major_load_with_stride, but with the PtrT argument itself 
begin a pointer type.
  template 
  matrix_t::type, R, C> 
column_major_load_with_stride2(PtrT Ptr) {
  return __builtin_matrix_column_major_load(Ptr, R, C, S);
  }
  
  void call_column_major_load_with_stride2(float *Ptr) {
  matrix_t m = column_major_load_with_stride2(Ptr);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenObjC/matrix-type-builtins.m
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins.cpp

Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -39,3 +39,65 @@
   Mat3.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
+
+constexpr int constexpr1() { return 1; }
+constexpr int constexpr_neg1() { return -1; }
+
+void test_column_major_load_constexpr(unsigned *Ptr) {
+  (void)__builtin_matrix_column_major_load(Ptr, 2, 2, constexpr1());
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+  (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]}}
+}
+
+struct IntWrapper {
+  operator int() {
+return 1;
+  }
+};
+
+void test_column_major_load_wrapper(unsigned *Ptr, IntWrapper ) {
+  (void)__builtin_matrix_column_major_load(Ptr, W, 2, 2);
+  // expected-error@-1 {{row argument must be a constant unsigned integer expression}}
+  (void)__builtin_matrix_column_major_load(Ptr, 2, W, 2);
+  // expected-error@-1 {{column argument must be a constant unsigned 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 263879.
fhahn edited the summary of this revision.
fhahn added a comment.

ping. Simplify code, extend tests. This should now be ready for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/matrix-type-builtins.c
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/Sema/matrix-type-builtins.c
  clang/test/SemaCXX/matrix-type-builtins.cpp

Index: clang/test/SemaCXX/matrix-type-builtins.cpp
===
--- clang/test/SemaCXX/matrix-type-builtins.cpp
+++ clang/test/SemaCXX/matrix-type-builtins.cpp
@@ -32,3 +32,27 @@
   Mat1.value = transpose(Mat2);
   // expected-note@-1 {{in instantiation of function template specialization 'transpose' requested here}}
 }
+
+template 
+typename MyMatrix::matrix_t column_major_load(MyMatrix , EltTy0 *Ptr) {
+  char *v1 = __builtin_matrix_column_major_load(Ptr, 9, 4, 10);
+  // expected-error@-1 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-2 {{cannot initialize a variable of type 'char *' with an rvalue of type 'unsigned int __attribute__((matrix_type(9, 4)))'}}
+  // expected-error@-3 {{cannot initialize a variable of type 'char *' with an rvalue of type 'float __attribute__((matrix_type(9, 4)))'}}
+
+  return __builtin_matrix_column_major_load(Ptr, R0, C0, R0);
+  // expected-error@-1 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(5, 5)))') with an rvalue of type 'unsigned int __attribute__((matrix_type(2, 3)))'}}
+  // expected-error@-2 {{cannot initialize return object of type 'typename MyMatrix::matrix_t' (aka 'unsigned int __attribute__((matrix_type(2, 3)))') with an rvalue of type 'float __attribute__((matrix_type(2, 3)))'}}
+}
+
+void test_column_major_loads_template(unsigned *Ptr1, float *Ptr2) {
+  MyMatrix Mat1;
+  Mat1.value = column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+  column_major_load(Mat1, Ptr1);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+
+  MyMatrix Mat2;
+  Mat1.value = column_major_load(Mat2, Ptr2);
+  // expected-note@-1 {{in instantiation of function template specialization 'column_major_load' requested here}}
+}
Index: clang/test/Sema/matrix-type-builtins.c
===
--- clang/test/Sema/matrix-type-builtins.c
+++ clang/test/Sema/matrix-type-builtins.c
@@ -15,3 +15,48 @@
   __builtin_matrix_transpose("test");
   // expected-error@-1 {{first argument must be a matrix}}
 }
+
+struct Foo {
+  unsigned x;
+};
+
+void column_major_load(float *p1, int *p2, _Bool *p3, struct Foo *p4) {
+  sx5x10_t a1 = __builtin_matrix_column_major_load(p1, 5, 11, 5);
+  // expected-error@-1 {{initializing 'sx5x10_t' (aka 'float __attribute__((matrix_type(5, 10)))') with an expression of incompatible type 'float __attribute__((matrix_type(5, 11)))'}}
+  sx5x10_t a2 = __builtin_matrix_column_major_load(p1, 5, 9, 5);
+  // expected-error@-1 {{initializing 'sx5x10_t' (aka 'float __attribute__((matrix_type(5, 10)))') with an expression of incompatible type 'float __attribute__((matrix_type(5, 9)))'}}
+  sx5x10_t a3 = __builtin_matrix_column_major_load(p1, 6, 10, 6);
+  // expected-error@-1 {{initializing 'sx5x10_t' (aka 'float __attribute__((matrix_type(5, 10)))') with an expression of incompatible type 'float __attribute__((matrix_type(6, 10)))'}}
+  sx5x10_t a4 = __builtin_matrix_column_major_load(p1, 4, 10, 4);
+  // expected-error@-1 {{initializing 'sx5x10_t' (aka 'float __attribute__((matrix_type(5, 10)))') with an expression of incompatible type 'float __attribute__((matrix_type(4, 10)))'}}
+  sx5x10_t a5 = __builtin_matrix_column_major_load(p1, 6, 9, 6);
+  // expected-error@-1 {{initializing 'sx5x10_t' (aka 'float __attribute__((matrix_type(5, 10)))') with an expression of incompatible type 'float __attribute__((matrix_type(6, 9)))'}}
+  sx5x10_t a6 = __builtin_matrix_column_major_load(p2, 5, 10, 6);
+  // expected-error@-1 {{initializing 'sx5x10_t' (aka 'float __attribute__((matrix_type(5, 10)))') with an expression of incompatible type 'int __attribute__((matrix_type(5, 10)))'}}
+
+  sx5x10_t a7 = __builtin_matrix_column_major_load(p1, 5, 10, 3);
+  // expected-error@-1 {{stride must be greater or equal to the number of rows}}
+
+  sx5x10_t a8 = __builtin_matrix_column_major_load(p3, 5, 10, 6);
+  // expected-error@-1 {{first 

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang (WIP).

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon question-circle color=gray} clang-tidy: unknown.

{icon question-circle color=gray} clang-format: unknown.

Build artifacts 
: 
diff.json 
,
 console-log.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72781



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang (WIP).

[Florian Hahn via Phabricator via cfe-commits]

fhahn created this revision.
Herald added a subscriber: tschuett.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72781

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-matrix.c

Index: clang/test/CodeGen/builtin-matrix.c
===
--- clang/test/CodeGen/builtin-matrix.c
+++ clang/test/CodeGen/builtin-matrix.c
@@ -271,4 +271,23 @@
 }
 // CHECK: declare <25 x double> @llvm.matrix.transpose.v25f64(<25 x double>, i32 immarg, i32 immarg) [[READNONE]]
 
+void column_load1(dx5x5_t *a, double *b) {
+  *a = __builtin_matrix_column_load(b, 5, 5, 10);
+
+  // CHECK-LABEL: @column_load1(
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%a.addr = alloca [25 x double]*, align 8
+  // CHECK-NEXT:%b.addr = alloca double*, align 8
+  // CHECK-NEXT:store [25 x double]* %a, [25 x double]** %a.addr, align 8
+  // CHECK-NEXT:store double* %b, double** %b.addr, align 8
+  // CHECK-NEXT:%0 = load double*, double** %b.addr, align 8
+  // CHECK-NEXT:%matrix = call <25 x double> @llvm.matrix.columnwise.load.v25f64.p0f64(double* %0, i32 10, i32 5, i32 5)
+  // CHECK-NEXT:%1 = load [25 x double]*, [25 x double]** %a.addr, align 8
+  // CHECK-NEXT:%2 = bitcast [25 x double]* %1 to <25 x double>*
+  // CHECK-NEXT:store <25 x double> %matrix, <25 x double>* %2, align 8
+  // CHECK-NEXT:ret void
+}
+// CHECK: declare <25 x double> @llvm.matrix.columnwise.load.v25f64.p0f64(double*, i32, i32 immarg, i32 immarg) [[READONLY:#[0-9]]]
+
 // CHECK: attributes [[READNONE]] = { nounwind readnone speculatable willreturn }
+// CHECK: attributes [[READONLY]] = { nounwind readonly willreturn }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1619,6 +1619,7 @@
   case Builtin::BI__builtin_matrix_subtract:
   case Builtin::BI__builtin_matrix_multiply:
   case Builtin::BI__builtin_matrix_transpose:
+  case Builtin::BI__builtin_matrix_column_load:
 if (!getLangOpts().EnableMatrix) {
   Diag(TheCall->getBeginLoc(), diag::err_builtin_matrix_disabled);
   return ExprError();
@@ -1636,6 +1637,8 @@
   return SemaBuiltinMatrixMultiplyOverload(TheCall, TheCallResult);
 case Builtin::BI__builtin_matrix_transpose:
   return SemaBuiltinMatrixTransposeOverload(TheCall, TheCallResult);
+case Builtin::BI__builtin_matrix_column_load:
+  return SemaBuiltinMatrixColumnLoadOverload(TheCall, TheCallResult);
 default:
   llvm_unreachable("All matrix builtins should be handled here!");
 }
@@ -15530,3 +15533,121 @@
   TheCall->setType(ResultType);
   return CallResult;
 }
+
+ExprResult Sema::SemaBuiltinMatrixColumnLoadOverload(CallExpr *TheCall,
+ ExprResult CallResult) {
+  // Must have exactly four operands
+  // 1: Pointer to data
+  // 2: Rows (constant)
+  // 3: Columns (constant)
+  // 5: Stride
+
+  // Operands have very similar semantics to glVertexAttribPointer from OpenGL.
+  // Instead of the attribute index, it is a pointer to the memory that is being
+  // loaded from Instead of size, we need the rows and columns. Note that these
+  // must be constant to construct the matrix type.
+
+  if (checkArgCount(*this, TheCall, 4))
+return ExprError();
+
+  Expr *DataExpr = TheCall->getArg(0);
+  Expr *RowsExpr = TheCall->getArg(1);
+  Expr *ColsExpr = TheCall->getArg(2);
+  Expr *StrideExpr = TheCall->getArg(3);
+
+  unsigned Rows = 0;
+  unsigned Cols = 0;
+
+  if (!(DataExpr->getType()->isPointerType() ||
+DataExpr->getType()->isArrayType())) {
+Diag(DataExpr->getBeginLoc(), diag::err_builtin_matrix_pointer_arg)
+<< 0 << 0;
+  }
+
+  bool ArgError = false;
+  // get the matrix dimensions
+  {
+llvm::APSInt Value(32);
+SourceLocation RowColErrorPos;
+
+if (!RowsExpr->isIntegerConstantExpr(Value, Context, )) {
+  Diag(RowsExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+  << 0 << 1;
+  ArgError = true;
+} else
+  Rows = Value.getZExtValue();
+
+if (!ColsExpr->isIntegerConstantExpr(Value, Context, )) {
+  Diag(ColsExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+  << 1 << 1;
+  ArgError = true;
+} else
+  Cols = Value.getZExtValue();
+  }
+  if (!StrideExpr->getType()->isIntegralType(Context)) {
+Diag(StrideExpr->getBeginLoc(), diag::err_builtin_matrix_scalar_int_arg)
+<< 3 << 1;
+ArgError = true;
+  }
+  if (ArgError)
+return ExprError();
+
+  QualType ElementType;
+
+  if (const PointerType *PTy = dyn_cast(DataExpr->getType())) {
+ElementType = PTy->getPointeeType();
+  } else if (const ArrayType *ATy = dyn_cast(DataExpr->getType())) {