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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


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


================
Comment at: clang/lib/Sema/SemaExpr.cpp:4707
+      return false;
+    }
+
----------------
rjmccall wrote:
> 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.
> 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.

Yes I updated the code here to use the same conversion function and added tests for conversion operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76791