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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1642
+  return cast<ConstantMatrixType>(Ty.getCanonicalType());
+};
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1920
+      return ExprError();
+    }
+
----------------
I don't think there's a need for this explicit check, since you're going to require the argument to have matrix type.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72778