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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4650
+      (Base->isTypeDependent() || RowIdx->isTypeDependent() ||
+       (ColumnIdx && ColumnIdx->isTypeDependent())))
+    return new (Context) MatrixSubscriptExpr(Base, RowIdx, ColumnIdx,
----------------
fhahn wrote:
> 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 :)
Placeholders actually need to be handled in the `Build` function — they can come up in template instantiation, too.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4645
+  // Separating the index expressions by parenthesis is not allowed.
+  if (isa<ParenExpr>(Base) && Base->getType()->isSpecificPlaceholderType(
+                                  BuiltinType::IncompleteMatrixIdx)) {
----------------
Don't check for `ParenExpr` specifically; there are other expressions that are handled the same way, like `_Generic`.  You need to check for `!isa<MatrixSubscriptExpr>`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4655
+  // type, in which case the overload resolution for the operator overload
+  // should get the first crack at the overload.
+
----------------
Overload placeholder types are used for things like `&foo` where `foo` is the name of an overloaded function.  The places that resolve only non-overload placeholder types are doing so in order to leave room for overload resolution to resolve the overload later, e.g. as part of non-builtin operator handling.  `operator[]` is like `operator()`: non-builtin operators are only considered when the base has class type.  Since you already know that the base type is a matrix type, you know that you're using your standard rules, and your standard rules have no way to resolve overloads in the index types — correctly, since indexes can't be functions or member pointers.

tl;dr: You can (and should) resolve all placeholders here, not just non-overload placeholders.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791