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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D99037#2667484 <https://reviews.llvm.org/D99037#2667484>, @SaurabhJha wrote:

> Addressed most of the comments. I couldn't understand "..would also be good to have C++ tests that test casting with matrix types where some of the dimensions are template arguments...". When I tried this
>
> """
> cx4x4 m1;
>
> (void)(ix4x4)m1
> """
>
> it gave me the error
> """
> C-style cast from 'cx4x4' (aka 'char __attribute__((matrix_type(4, 4)))') to 'ix4x4' (aka 'int __attribute__((matrix_type(4, 4)))') is not allowed
> """

Oh, that's interesting, I was assuming the code-paths would be the same. I was thinking about C++ specific test cases like

  template <typename X>
  using matrix_t = X __attribute__((matrix_type(4, 4)));
  
  matrix_t<int> foo(matrix_t<float> x) {
    return (matrix_t<int>)x;
  }

> should I address it as part of this patch?

I don't think we necessarily need to implement them in this patch, unless @rjmccall could think of any issues. IMO it would still be good to add tests to Sema to ensure we do not crash. We can then just update them once support for C++ is added.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8580
 
+def err_invalid_conversion_between_matrices : Error<
+  "invalid conversion between matrix type%diff{ $ and $|}0,1 of different "
----------------
I think in other places we are already using `matrixes`. Would be good to be consistent with the existing spelling I think (but I am no English expert)


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8585
+def err_invalid_conversion_between_matrix_and_non_matrix : Error<
+  "invalid conversion between matrix type %0 and non matrix type %1">;
+
----------------
I think for other messages we say `... is not allowed`. Perhaps this message should be similar? Also I'm not sure about reefing to non-matrix types. We might allow at least for conversion between arithmetic types and matrixes (broadcast). It might be OK to just drop the `non matrix` part?


================
Comment at: clang/include/clang/Sema/Sema.h:11707
 
+  // CheckMatrixCast - Check type constraints for matrices.
+  // We allow casting between matrices of the same dimensions i.e. when they
----------------
nit: for matrix casts?


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1316
+
+    llvm::Type *SrcElementTy = cast<llvm::VectorType>(SrcTy)->getElementType();
+
----------------
You could move the code generation part to `MatrixBuilder.h`?


================
Comment at: clang/lib/Sema/SemaCast.cpp:2916
 
   // Require the operand to be a scalar or vector.
+  if (!SrcType->isScalarType() && !SrcType->isVectorType() &&
----------------
fhahn wrote:
> ` ... or a matrix`?
nit: missing comma after `a scalar`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7348
 
+/// Are the two types matrix types and do they have the same dimensions i.e.
+/// and do they both have the same dimensions i.e. do they have the same number
----------------
>From the comment here it sounds like this function checks if the arguments are matrix types, but it asserts. Does the comment need updating?


It looks like the part after `i.e.` needs rewording also, as there's some duplication?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99037