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

Mon Jun 1 10:46:11 PDT 2020

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.

================
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:
> > 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.
> I now updated the code to force the index types to size_t in Sema and remove the zext generation here. If we have signed indices, we now generate sexts, which may require additional instructions down the road., which we would not require if we perform the math on the max bit width of the operands. But looking back now, I am not really sure if that would actually be safe in all cases. 
> 
> We  should probably just try to narrow the index types based on the number of vector elements as an IR optimization, if it becomes a problem.
It's UB if it's actually negative, right?  So the `sext` really shouldn't be a problem.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791