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:
> 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.
> Thanks. In that case, there should be a way to more directly query whether a
> particular expression is complete or not.
Done, I've added an isIncomplete method. I also added assertions using it in a
few places.
================
Comment at: clang/include/clang/Basic/Specifiers.h:159
+
+ /// A single matrix element of a matrix.
+ OK_MatrixElement
----------------
rjmccall wrote:
> 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`.
I've renamed it to OK_MatrixComponent and added a comment that a matrix
component is a single element of a matrix.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4238
+ case Expr::MatrixSubscriptExprClass:
+ llvm_unreachable("matrix subscript expressions not supported yet");
+
----------------
rjmccall wrote:
> 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.
> 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.
Thanks for the suggestion! I think it would be best to mirror the handling of
ExtVectorElementExpr and I *think* I found all the relevant places that needed
updating. I've added tests to take references/addresses with various
valid/invalid cases.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4556
+ if (base->getType()->isMatrixType())
+ return ActOnMatrixSubscriptExpr(S, base, idx, nullptr, rbLoc);
+
----------------
rjmccall wrote:
> 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]`.
Oh right! I've went back to add a ActOnMatrixSubscriptExpr, as there are a few
things that can go in there now, including rejecting `m[0,0]`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4583
+ auto BaseTy = base->getType();
+ if (BaseTy->isNonOverloadPlaceholderType()) {
IsMSPropertySubscript = isMSPropertySubscriptExpr(*this, base);
----------------
rjmccall wrote:
> This change is no longer necessary.
Dropped, thanks!
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4650
+ (Base->isTypeDependent() || RowIdx->isTypeDependent() ||
+ (ColumnIdx && ColumnIdx->isTypeDependent())))
+ return new (Context) MatrixSubscriptExpr(Base, RowIdx, ColumnIdx,
----------------
rjmccall wrote:
> 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.
> 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:
Done, I've added a ActOnMatrixSubscriptExpr and added code to deal with
placeholder types there.
> 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.
Done, I initially thought it might be good to still raise an error if the row
index was invalid, but given that it's not a valid expression anyways that
probably is not too helpful anyways. Doing the checks on the complete
expression simplifies things a bit :)
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
