rjmccall added inline comments.
================ Comment at: clang/lib/CodeGen/CGExpr.cpp:1900 + assert(!LV.isMatrixElt() && + "loads of matrix element LValues should be handled elsewhere"); assert(LV.isBitField() && "Unknown LValue type!"); ---------------- This should be handled here or else a whole lot of unusual cases are going to blow up on you — compound operators and so on. ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3787 + ColIdx->getType()->getScalarSizeInBits()); + llvm::Type *IntTy = llvm::IntegerType::get(getLLVMContext(), MaxWidth); + RowIdx = Builder.CreateZExt(RowIdx, IntTy); ---------------- You should be able to assert that these have been coerced to the right type by Sema (probably size_t or ptrdiff_t or something). ================ 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. + ---------------- fhahn wrote: > 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 > ``` Yeah, you have to resolve some placeholders before you can check whether the base is a matrix type, and you can't resolve MS properties because the property access actually merges with the subscript in some cases. I think you may need to resolve placeholders in base even in CreateBuiltinMatrixSubscriptExpr to handle template instantiation right. The test case would be something where the matrix expression is non-dependently-typed but loaded from an ObjC property — we might need to redundantly resolve placeholders when rebuilding expressions in the instantiation. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:4553 + if (!MaybeMatrixSubscript.isUnset()) + return MaybeMatrixSubscript; + ---------------- I would prefer that the checks are inlined here, since they're very likely to not trigger. 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
