rjmccall added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15051
+
+ Expr *Arg = TheCall->getArg(0);
+ if (!Arg->getType()->isConstantMatrixType()) {
----------------
fhahn wrote:
> 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.
Oh, I'm sorry, I gave you poor advice: you do need to handle placeholders, but
more generally than that, you need an r-value. Since you aren't doing any
further conversions, you just need to call DefaultLValueConversion, which will
also handle placeholders for you.
You will also need to store the checked argument back into the call, which you
can only really test with an IRGen test. This is one of the few places where
we do actually directly mutate the AST.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1918
+ switch (BuiltinID) {
+ case Builtin::BI__builtin_matrix_transpose:
+ return SemaBuiltinMatrixTransposeOverload(TheCall, TheCallResult);
----------------
I didn't notice this before, but I think a single level of switch is fine;
there's probably nothing common about matrix builtins that you're going to want
to handle like this.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15063
+ // matrix type.
+ auto *MType = Matrix->getType()->getAs<ConstantMatrixType>();
+ QualType ResultType = Context.getConstantMatrixType(
----------------
Please call `getAs` once and then check the result above instead of calling
`isConstantMatrixType()` (which does most of the same work as `getAs`).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72778/new/
https://reviews.llvm.org/D72778
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
