rjmccall added a comment. In D76791#2064434 <https://reviews.llvm.org/D76791#2064434>, @fhahn wrote:
> In D76791#2063926 <https://reviews.llvm.org/D76791#2063926>, @rjmccall wrote: > > > > Yes at the moment I think we want to limit element wise > > > accesses/modifications to go through the access operator only, to > > > guarantee we can rely on the vector forms in codegen. > > > > > > Additionally I think at least initially we want to avoid handing out > > > pointers to elements, as it may be tempting to use for pointer-based > > > accesses to subsequent elements. I am a bit worried that allowing that > > > would muddy the waters a bit and may lead to interpreting matrix types as > > > similar to arrays, instead of single value types with restricted element > > > access. Does that make sense? > > > > It does make sense, although it does make it very important that code > > generation narrows the relevant particular load/extractvalue and > > load/insertvalue/store sequences, or else performance and code size will be > > completely unacceptable for large matrices. > > > Yes, dealing with large vectors is currently a weakness of LLVM > unfortunately. But breaking up operations on large vectors in general is > something we are planning on implementing as part of the matrix lowering. And > I think even if we do not allow taking addresses, we could still just > load/store a single element with the index operator, if required (maybe also > based on the actual size of the matrix). And if we discover that taking > addresses is really beneficial to users, it should be relatively straight > forward to allow it in a backwards-compatible way I think/ Yeah, if you ever decide you want that IR pattern, it should be painless to make IRGen emit it. You might even consider doing it at -O0 if the backend wouldn't otherwise be narrowing then. (That doesn't need to be in this patch, of course.) ================ Comment at: clang/lib/CodeGen/CGExpr.cpp:3787 + ColIdx->getType()->getScalarSizeInBits()); + llvm::Type *IntTy = llvm::IntegerType::get(getLLVMContext(), MaxWidth); + RowIdx = Builder.CreateZExt(RowIdx, IntTy); ---------------- fhahn wrote: > rjmccall wrote: > > 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). > Hm I don't think that's currently happening. Is that required? It seems a bit > unfortunate if we would have to force to wider bit-widths than necessary for > the given index expressions. Can you explain what cost you're worried about? I don't understand what the benefit of using a smaller IR type for the indexes is, and usually it's better to have stronger invariants coming out of Sema. ================ 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: > > 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. > > 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 that should be happening in the latest version, CheckPlaceholderExpr > is used on Base, RowIdx and ColumnIdx. Yes, you're right. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:4707 + return false; + } + ---------------- Somewhere in here would be where you'd do the index type conversion, FWIW. I don't know if you want to allow a class with a conversion operator to an integer type, but that function you added for +/- should do it. 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
