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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:2648
+/// MatrixSubscriptExpr - Matrix subscript expression for the MatrixType
+/// extension.
+class MatrixSubscriptExpr : public Expr {
----------------
rjmccall wrote:
> 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.
> 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.

My intuition was that the matrix subscript operator would be a single operator with 3 arguments. Allowing things like (matrix[I])[j] has potential for confusion I think, as there is no way to create an expression just referencing a row/column on its own. (responded in more details at the SemaExpr.cpp comment).

> What expression node do you use for the intermediate subscript expression? You should talk about this in the doc comment.

Intermediate subscript expressions are represented as 'incomplete' MatrixSubscriptExpr (type is MatrixIncompleteIdx, ColumnIdx is nullptr). I've updated the comment.


================
Comment at: clang/include/clang/Basic/Specifiers.h:159
+
+    /// A single matrix element of a matrix.
+    OK_MatrixElement
----------------
rjmccall wrote:
> redundancy
I suppose no comment is sufficient?


================
Comment at: clang/include/clang/Sema/Sema.h:4907
+  ExprResult ActOnMatrixSubscriptExpr(Scope *S, Expr *Base, Expr *RowIdx,
+                                      Expr *ColumnIdx, SourceLocation RBLoc);
+
----------------
rjmccall wrote:
> 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.
Thanks, I wasn't sure about the actual distinction. I've renamed it to BuildMatrixSubscriptExpr now.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4238
+  case Expr::MatrixSubscriptExprClass:
+    llvm_unreachable("matrix subscript expressions not supported yet");
+
----------------
rjmccall wrote:
> 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]; }
> ```
Thanks, I've updated the code as suggested and added the test.  I had to adjust the test slightly (adding a call to matrix_subscript).

This test is quite interesting, because it accidentally another issue. Matrix element subscripts are treated as Lvalues currently, but the code currently does not support taking the address of matrix elements. `decltype(m[r][c])` will be `& double` and if you add `return m[r][c];` the issue is exposed.

I am not sure how to best address this, short of computing the address of the element in memory directly. Do you have any suggestions?

I think for some vector types, we currently mis-compile here as well. For example,

```
using double4 = double __attribute__((ext_vector_type(4)));

template <class R>
auto matrix_subscript(const double4 m, R r) -> decltype(m[r]) { return m[r]; }
```

produces the IR below. Note the `  ret double* %ref.tmp`, where `%ref.tmp` is an alloca.

```
define linkonce_odr nonnull align 8 dereferenceable(8) double* @_Z16matrix_subscriptIiEDTixfpK_fp0_EDv4_dT_(<4 x double>* byval(<4 x double>) align 16 %0, i32 %r) #0 {
entry:
  %m.addr = alloca <4 x double>, align 16
  %r.addr = alloca i32, align 4
  %ref.tmp = alloca double, align 8
  %m = load <4 x double>, <4 x double>* %0, align 16
  store <4 x double> %m, <4 x double>* %m.addr, align 16
  store i32 %r, i32* %r.addr, align 4
  %1 = load <4 x double>, <4 x double>* %m.addr, align 16
  %2 = load i32, i32* %r.addr, align 4
  %vecext = extractelement <4 x double> %1, i32 %2
  store double %vecext, double* %ref.tmp, align 8
  ret double* %ref.tmp
}
```


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4556
+  if (base->getType()->isMatrixType())
+    return ActOnMatrixSubscriptExpr(S, base, idx, nullptr, rbLoc);
+
----------------
rjmccall wrote:
> 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?
> 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.


Yes the way I think about the matrix subscript expression is in terms of a single operator with 3 arguments (`base[RowIdx][ColumnIdx]`), so something like `(a[i])[j]` should not be allowed, similar to things like `a([I])` not being allowed. 

> 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.

I've moved the handling of IncompleteMatrixIdx here (the previous position was due to the first version of the patch).

> Doing this before the C++2a comma-index check is intentional?

Not really. I've moved it after the check.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791