rjmccall added inline comments.
================
Comment at: clang/include/clang/AST/Expr.h:2648
+/// MatrixSubscriptExpr - Matrix subscript expression for the MatrixType
+/// extension.
+class MatrixSubscriptExpr : public Expr {
----------------
Oh, that's interesting. So you've changed this to flatten the component
expressions? I think that might be inconsistent with our usual
source-preservation goals unless you intend to restrict the intermediate base
expression to be an immediate subscript. That is, this is okay if you're going
to require the user to write `matrix[i][j]` and forbid `(matrix[i])[j]`, but if
you intend to allow the latter, you should preserve that structure here. You
can do that while still providing this API; you just have to implement
`getBase()` etc. by looking through parens, and you should have an accessor
which returns the syntactic base expression.
What expression node do you use for the intermediate subscript expression? You
should talk about this in the doc comment.
================
Comment at: clang/include/clang/Basic/Specifiers.h:159
+
+ /// A single matrix element of a matrix.
+ OK_MatrixElement
----------------
redundancy
================
Comment at: clang/include/clang/Sema/Sema.h:4907
+ ExprResult ActOnMatrixSubscriptExpr(Scope *S, Expr *Base, Expr *RowIdx,
+ Expr *ColumnIdx, SourceLocation RBLoc);
+
----------------
It'd be more conventional to call it `BuildMatrixSubscriptExpr`. You can do
all the semantic checks here and make `ActOnArraySubscriptExpr` handle the
syntactic checks. This is all assuming that you intend to impose the syntactic
restriction discussed above.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4238
+ case Expr::MatrixSubscriptExprClass:
+ llvm_unreachable("matrix subscript expressions not supported yet");
+
----------------
This is simple, you should just mangle them syntactically. `'ixix' <base
expression> <row expression> <column expression>`. Test case is
```
using double4x4 = double __attribute__((matrix_type(4,4)));
template <class R, class C>
auto matrix_subscript(double4x4 m, R r, C c) -> decltype(m[r][c]) {}
double test_matrix_subscript(double 4x4 m) { return m[3][2]; }
```
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4556
+ if (base->getType()->isMatrixType())
+ return ActOnMatrixSubscriptExpr(S, base, idx, nullptr, rbLoc);
+
----------------
You're not handling the case of a parenthesized MatrixSubscriptExpr. If that
should be an error, that's a reasonable language rule, but it should probably
be a better error than whatever you'll get by default.
And you might be specifically and problematically avoiding an error because of
the special treatment of `IncompleteMatrixIdx` below. I'd just pull that up
here and emit an error.
Doing this before the C++2a comma-index check is intentional?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76791/new/
https://reviews.llvm.org/D76791
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
