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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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,
----------------
rjmccall wrote:
> 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.
Move, thanks! I think we have to retain checking placeholder expressions for base below, as noted in the response below.


================
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)) {
----------------
rjmccall wrote:
> 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>`.
Done, thanks!


================
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.
+
----------------
rjmccall wrote:
> 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.
Thanks for the clarification. I moved the code to deal with placeholders to CreateBuiltinMatrixSubscriptExpr and removed the non-overload restriction there.

I think we still need to keep dealing with placeholders in `Base` below, to ensure we do not miss that the base is actually a matrix type, e.g. to support. It seems it is enough to skip` isMSPropertySubscriptExpr` there (without that restriction, some non-matrix-type codegen tests start to fail. Does that make sense?

```
__attribute__((objc_root_class))
@interface MatrixValue
@property double4x4 value;
@end
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791