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

[John McCall via Phabricator via llvm-commits]

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