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

[John McCall via Phabricator via llvm-commits]

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