[PATCH] D101762: [Matrix] Fold the transpose into the matmul operand used to fetch scalars

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1018
   /// Compute \p Result += \p A * \p B for input matrices with left-associating
   /// addition.
   void emitMatrixMultiply(MatrixTy &Result, const MatrixTy &A,
----------------
Can you mentioned `scalar_matrix_transposed` in the comment?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1022
+                          IRBuilder<> &Builder, bool isTiled,
+                          bool scalar_matrix_transposed) {
     const unsigned VF = std::max<unsigned>(
----------------
Can you adjust the name to camel case so it matches the style elsewhere in the file (`isTiled` is also an odd one out)


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1356
                                 SmallPtrSetImpl<Instruction *> &FusedInsts) {
-    if (!FuseMatrix || !MatMul->hasOneUse() ||
-        MatrixLayout != MatrixLayoutTy::ColumnMajor || !DT)
+    if (!FuseMatrix)
       return;
----------------
I think the crashes/test failures shown for the pre-merge checks may be related to not bailing out early if `!DT`.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1361
 
+    auto *EltType = cast<VectorType>(MatMul->getType())->getElementType();
+    ShapeInfo LShape(MatMul->getArgOperand(2), MatMul->getArgOperand(3));
----------------
 Those changes seem quite isolated form the other code in the function. Might be worth to have them in a separate function and call it before `LowerMatrixMultiplyFused`? Otherwise I think it would be good to adjust the name/comment for the function.


================
Comment at: llvm/test/Transforms/LowerMatrixIntrinsics/multiply-left-transpose-row-major.ll:3
+
+; RUN: opt -lower-matrix-intrinsics -matrix-default-layout=row-major -S < %s | FileCheck --check-prefix=RM %s
+
----------------
no need to use the `RM` prefix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101762