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

[Florian Hahn via Phabricator via llvm-commits]

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