[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

[John McCall via Phabricator via cfe-commits]

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