[PATCH] D131125: [Matrix] Add special case dot product lowering

[Vir Narula via Phabricator via llvm-commits]

virnarula added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1217
+                       FastMathFlags FMF) {
+    ShapeInfo LShape(MatMul->getArgOperand(2), MatMul->getArgOperand(3));
+    ShapeInfo RShape(MatMul->getArgOperand(3), MatMul->getArgOperand(4));
----------------
fhahn wrote:
> I think we should be able to just look up the shape of operands 0 and 1 in ShapeMap instead?
Using `ShapeMap` works when we load matrix inside the function. However, it doesn't if the matrix is passed in as an argument. As such, I have not implemented this.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1292
+    if (LHSBuiltinLoad) {
+      LHS =
+          Builder.CreateLoad(LHS->getType(), LHSBuiltinLoad->getArgOperand(0));
----------------
fhahn wrote:
> I am not sure if that's correct. Here we need to load a vector with 1 row and N columns. `llvm.matrix.column.load` may have a stride > 1 between columns, which is ignored. We should probably limit this to .`llvm.matrix.column.load` with a stride of 1 here for now. Needs a test case.
Most of our test cases didn't account for this (they used strides greater than 1). I am submitting an additional review that will update the test cases to use strides of 1 but keep some that don't as test cases.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1292
+    if (LHSBuiltinLoad) {
+      LHS =
+          Builder.CreateLoad(LHS->getType(), LHSBuiltinLoad->getArgOperand(0));
----------------
virnarula wrote:
> fhahn wrote:
> > I am not sure if that's correct. Here we need to load a vector with 1 row and N columns. `llvm.matrix.column.load` may have a stride > 1 between columns, which is ignored. We should probably limit this to .`llvm.matrix.column.load` with a stride of 1 here for now. Needs a test case.
> Most of our test cases didn't account for this (they used strides greater than 1). I am submitting an additional review that will update the test cases to use strides of 1 but keep some that don't as test cases.
https://reviews.llvm.org/D131444


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131125