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

Fri May 29 16:25:07 PDT 2020

fhahn added a comment.

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/

> The test changes in the other patch look great, thanks.

Great, I'll update them ASAP, although it takes a while. I am also submitting some inline-response that didn't got submitted when I updated the patch a few hours ago.

================
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!");
----------------
rjmccall wrote:
> 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.
Oh thanks for pointing me to those cases. Fixed and test cases added.

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3787
+                               ColIdx->getType()->getScalarSizeInBits());
+  llvm::Type *IntTy = llvm::IntegerType::get(getLLVMContext(), MaxWidth);
+  RowIdx = Builder.CreateZExt(RowIdx, IntTy);
----------------
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.

================
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:
> 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.

================
Comment at: clang/lib/Sema/SemaExpr.cpp:4553
+  if (!MaybeMatrixSubscript.isUnset())
+    return MaybeMatrixSubscript;
+
----------------
rjmccall wrote:
> I would prefer that the checks are inlined here, since they're very likely to not trigger.
Done, I've removed ActOnMatrixSubscriptExpr again.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791