[PATCH] D99037: [Matrix] Implement C-style explicit type conversions for matrix types

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

Thanks for the latest update! This basically looks good to me, with a few final nits!



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8584
+def err_invalid_conversion_between_matrixes : Error<
+  "conversion between matrix type%diff{ $ and $|}0,1 of different size is not allowed">;
+
----------------
nit: `matrix types`?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8586
+
+def err_invalid_conversion_between_matrix_and_type : Error<
+  "conversion between matrix type %0 and type %1 is not allowed">;
----------------
nit: `_and_incompatible_type`?


================
Comment at: clang/include/clang/Sema/Sema.h:11718
+  // invalid.
+  bool CheckMatrixCast(SourceRange R, QualType MatrixTy, QualType Tr,
+                       CastKind &Kind);
----------------
Can the name for `Tr` be improved? Perhaps `ToTy` or `TargetTy`?


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1219
+  } else {
+    assert(!SrcType->isMatrixType() && !DstType->isMatrixType());
+    SrcElementTy = SrcTy;
----------------
Thanks! Can you also add a message to the assert , like `&& "cannot cast between matrix and non-matrix types")`


================
Comment at: clang/lib/Sema/SemaCast.cpp:2932
+  if (DestType->getAs<MatrixType>() || SrcType->getAs<MatrixType>()) {
+    if (Self.CheckMatrixCast(OpRange, DestType, SrcType, Kind)) {
+      SrcExpr = ExprError();
----------------
nit: no braces.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7357
+
+  return (matSrcType->getNumRows() == matDestType->getNumRows() &&
+          matSrcType->getNumColumns() == matDestType->getNumColumns());
----------------
nit: No `( ...... )` required here.


================
Comment at: clang/test/CodeGen/matrix-cast.c:82
+
+void cast_unsigned_short_int_to_unsigned_int(unsigned_short_int_5x5 s, unsigned_int_5x5 i) {
+  // CHECK-LABEL: define{{.*}} void @cast_unsigned_short_int_to_unsigned_int(<25 x i16> %s, <25 x i32> %i)
----------------
I think this is still missing a test from unsigned to signed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99037