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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
