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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Sorry for the slow review; I'm getting crushed with review requests.



================
Comment at: clang/lib/AST/Expr.cpp:3859
+
+  auto *SubscriptE = dyn_cast<ArraySubscriptExpr>(this);
+  return SubscriptE
----------------
You need to `IgnoreParens()` here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:7768
+  if (E->getBase()->getMatrixFromIndexExpr(Info.getLangOpts().MatrixTypes))
+    return false;
+
----------------
This can also occur in the other evaluators due to indexing into an r-value.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5326
+    VK = VK_LValue;
+    OK = OK_VectorComponent;
   } else if (RHSTy->isArrayType()) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791