[PATCH] D76325: [Matrix] Add option to use row-major matrix layout as default.

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

> TODO: additional tests.

Is this WIP?



================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:312
+
+    /// Extract a column vector of \p NumElts starting at index (\p I, \p J)
+    /// from the matrix \p LM represented as a vector of column vectors.
----------------
column vector?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:784
+    for (unsigned C = 0, E = Shape.getNumVectors(); C < E; ++C) {
+      Value *GEP = computeColumnAddr(EltPtr, Builder.getInt32(C), Stride,
+                                     Shape.getStride(), VType->getElementType(),
----------------
is this computeColumnAddr or computeVectorAddr now?


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:983-987
   /// Compute Res += A * B for tile-sized matrices with left-associating
   /// addition.
   void emitChainedMatrixMultiply(MatrixTy &Result, const MatrixTy &A,
                                  const MatrixTy &B, bool AllowContraction,
                                  IRBuilder<> &Builder, bool isTiled) {
----------------
This interface got me confused in particular in the context of this patch.  Comment says tile-sized but then there is a flag whether it's tiled at all.  Anyway we can improve this?

Sounds like this should be just called emitMatrixMultiply?  You might want to insert a patch before this one that fixes this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76325