[PATCH] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

[Florian Hahn via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1642
+  return cast<ConstantMatrixType>(Ty.getCanonicalType());
+};
+
----------------
rjmccall wrote:
> Unnecessary semicolon.  I think it's probably clearer just to `castAs<ConstantMatrixType>()` inline in the code rather than introducing this trivial wrapper for it.  We generally treat getAs/castAs as idiomatically implying the result type strong enough to permit `auto`, just like `dyn_cast`, so it'll probably even be more compact than this overall.
Sounds good, I dropped it and replaced it with getAs


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15051
+
+  Expr *Arg = TheCall->getArg(0);
+  if (!Arg->getType()->isConstantMatrixType()) {
----------------
rjmccall wrote:
> When a builtin has custom type-checking (`t`), you need to handle placeholders in the operands yourself, just like you would for an operator.
I added placeholder handling and added additional tests.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:15060
+  ConstantMatrixType const *MType =
+      cast<ConstantMatrixType const>(Arg->getType().getCanonicalType());
+  QualType ResultType = Context.getConstantMatrixType(
----------------
rjmccall wrote:
> Don't canonicalize here, and you don't need to include `const` anyway.
dropped, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72778