fhahn marked an inline comment as done.
fhahn added inline comments.
================
Comment at: clang/lib/AST/Expr.cpp:3859
+
+ auto *SubscriptE = dyn_cast<ArraySubscriptExpr>(this);
+ return SubscriptE
----------------
rjmccall wrote:
> You need to `IgnoreParens()` here.
This code is now gone.
================
Comment at: clang/lib/AST/ExprConstant.cpp:7768
+ if (E->getBase()->getMatrixFromIndexExpr(Info.getLangOpts().MatrixTypes))
+ return false;
+
----------------
rjmccall wrote:
> This can also occur in the other evaluators due to indexing into an r-value.
This code is not required any longer.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5326
+ VK = VK_LValue;
+ OK = OK_VectorComponent;
} else if (RHSTy->isArrayType()) {
----------------
rjmccall wrote:
> It should have the value kind of its base. `get_matrix_rvalue()[0][0]` is
> still an r-value, so you shouldn't allow it to be assigned to. Needs tests.
>
> I'm fine with re-using `OK_VectorComponent` here, but you should (1) rename
> it to `OK_VectorOrMatrixComponent` and (2) take the opportunity to audit the
> places that use it to make sure they do something sensible. I expect you'll
> need to at least update some diagnostics about e.g. disallowing taking the
> address of the expression.
>
> I think you should build a new expression node instead of reusing
> `ArraySubscriptExpr`, since basically none of the code that looks for an
> `ArraySubscriptExpr` will do the right thing for your operation. That will
> also allow you to avoid the "is this actually enabled" check, since you'll
> only see this node when your feature is enabled. If you're feeling generous,
> you could move vector subscripting to this new node as well and leave
> `ArraySubscriptExpr` exclusively for the standard (or dependent) cases.
> It should have the value kind of its base. get_matrix_rvalue()[0][0] is
> still an r-value, so you shouldn't allow it to be assigned to. Needs tests.
added tests to Sema/matrix-type-operators.s and
SemaCXX/matrix-type-operators.cpp.
> I'm fine with re-using OK_VectorComponent here, but you should (1) rename it
> to OK_VectorOrMatrixComponent and (2) take the opportunity to audit the
> places that use it to make sure they do something sensible. I expect you'll
> need to at least update some diagnostics about e.g. disallowing taking the
> address of the expression.
I've introduced a new OK_MatrixElement, because I don't think there is enough
information to distinguish between vectors/matrixes where it is used. Also
added a reinterpret_cast test.
> I think you should build a new expression node instead of reusing
> ArraySubscriptExpr, since basically none of the code that looks for an
> ArraySubscriptExpr will do the right thing for your operation. That will also
> allow you to avoid the "is this actually enabled" check, since you'll only
> see this node when your feature is enabled. If you're feeling generous, you
> could move vector subscripting to this new node as well and leave
> ArraySubscriptExpr exclusively for the standard (or dependent) cases.
I've added a new MatrixSubscriptExpr. That helps to simplify the code in a
couple of places. ActOnArraySubscriptExpr calls ActOnMatrixSubscriptExpr if we
can identify the base as matrix type or MatrixSubscriptExpr. I initially tried
to move the ActOnMatrixSubscriptExpr call to the parse step, but that does not
work in the presence of dependent base types I think. Until we instantiate the
dependent types, there's no way to distinguish between
ArraySubscriptExpr/MatrixSubscriptExpr I think and it is best to treat them as
unanalyzed ArraySubscriptExpr until then.
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
