[PATCH] D80673: [llvm] [MatrixIntrinsics] Add row-major support for llvm.matrix.transpose

[Aart Bik via Phabricator via llvm-commits]

aartbik added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:1300
+      // Column-major tranposition.
+      for (unsigned Row = 0; Row < ArgShape.NumRows; ++Row) {
+        // Build a single column vector for this row. First initialize it.
----------------
fhahn wrote:
> I think it might be possible to share most of the transpose implementation for both row/column major transpose, as we are iterating over the indices of the second dimension while extracting from the vectors of the first dimension in both cases.
> 
> Instead of using `ArgShape.NumRows`, we could just use the second dimension (`InputMatrix.isColumnMajor() : ArgShape.NumRows : ArgShape.NumColumns` and instead of iterating over columns/rows we could just iterate over the vectors of the leading dimension (`InputMatrix.vectors()`) and instead of `ArgShape.NumColumns/NumRows`, we could use `ArgShape.getNumVectors()`.
That is a good idea. Fits nicely with the provided utilities. Thanks!


================
Comment at: llvm/test/Transforms/LowerMatrixIntrinsics/pr46085-row.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -lower-matrix-intrinsics -matrix-default-layout=row-major -S < %s | FileCheck  %s
----------------
fhahn wrote:
> for multiply, we just have row/column major test variants in `multiply-i32-row-major.ll`/`multiply-i32.ll` and so on. For consistency, it might be slightly preferable to extend the existing transpose-*.ll tests (if the cases in the new test are not covered) and call the row-major equivalent `transpose-*-row-major.ll`.
Sounds good. I removed the pr* tests and used the existing transpose tests for RM. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80673