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

[John McCall via Phabricator via llvm-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 {
----------------
fhahn wrote:
> 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.
> 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).

I think it's a perfectly reasonable language design to disallow parentheses here; it just wasn't obvious at first that that was what you were intending to do.

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

Thanks.  In that case, there should be a way to more directly query whether a particular expression is complete or not.


================
Comment at: clang/include/clang/Basic/Specifiers.h:159
+
+    /// A single matrix element of a matrix.
+    OK_MatrixElement
----------------
fhahn wrote:
> rjmccall wrote:
> > redundancy
> I suppose no comment is sufficient?
A comment is a good idea; I was just pointing out that you'd written "a matrix element of a matrix".

If you're intending to allow more complex matrix components in the future (e.g. row/column projections), consider going ahead and calling this `OK_MatrixComponent`. 


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4238
+  case Expr::MatrixSubscriptExprClass:
+    llvm_unreachable("matrix subscript expressions not supported yet");
+
----------------
fhahn wrote:
> 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
> }
> ```
> 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).

Er, yes, I did mean to include one of those. :)

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

Well, there's two levels of this.

First, you *could* make matrix elements ordinary l-values — it's not like you have any interest in ever not storing them separately, right?  Vector components use a special l-value kind specifically to prevent addressability, probably for reasons related to the swizzle projections.  Although, saying that, I'm not sure you can assign to the swizzle projections, in which case I'm not really sure why vectors need a special object kind except to forbid taking the address for its own sake.  The only problem I can see with allowing element l-values to be ordinary l-values is that the code coming out of IRGen after you assign to one isn't going to be quite so tidily mem2reg'able as it is with the vector-style code generation.  Of course, you could peephole that in IRGen if it's important.  And if you want e.g. projected column matrices to be non-addressable in the future (which I assume you do?), you can always add a special object kind for those when you add them.

If you do want to restrict taking the address of an matrix component l-value, that should be a straightforward extra restriction in the places where we already forbid taking the address of a bit-field.  You'll need the same restriction in reference-binding, of course.  You could make `decltype` not infer a reference type for your l-values if you want, although I believe `decltype` does infer a reference type for bit-fields.

Your example is unfortunately not a miscompile, it's just a combination of terrible language features.  The returned reference is bound to a materialized temporary because vector elements aren't addressable.  You wouldn't be able to do that if `m` weren't `const` because the return type would become `double &`, which can't bind to a temporary.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4556
+  if (base->getType()->isMatrixType())
+    return ActOnMatrixSubscriptExpr(S, base, idx, nullptr, rbLoc);
+
----------------
fhahn wrote:
> 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.
> 
> 
> Not really. I've moved it after the check.

Okay.  However, you should consider going ahead and making it an error for matrix subscripts, just in case you want that syntax to mean something else in the future.  It will also let you immediately catch people trying to use `m[0,0]` instead of `m[0][0]`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4583
+  auto BaseTy = base->getType();
+  if (BaseTy->isNonOverloadPlaceholderType()) {
     IsMSPropertySubscript = isMSPropertySubscriptExpr(*this, base);
----------------
This change is no longer necessary.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4650
+      (Base->isTypeDependent() || RowIdx->isTypeDependent() ||
+       (ColumnIdx && ColumnIdx->isTypeDependent())))
+    return new (Context) MatrixSubscriptExpr(Base, RowIdx, ColumnIdx,
----------------
Checking dependence is actually just as cheap as checking for C++, there's no real need to gate.  But you need to check for placeholder types in the index operands before doing these type checks.  The best test case here is an Objective-C property reference, something like:

```
__attribute__((objc_root_class))
@interface IntValue
@property int value;
@end

double test(double4x4 m, IntValue *iv) {
  return m[iv.value][iv.value];
}
```

Also, I would suggest not doing any checking on incomplete matrix expressions; just notice that it's still incomplete, build the expression, and return.  You can do the checking once when you have all the operands together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791