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

Mon Mar 30 05:55:20 PDT 2020

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

================
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(),
----------------
anemet wrote:
> is this computeColumnAddr or computeVectorAddr now?
Fix and also in various other places.

================
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) {
----------------
anemet wrote:
> 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.
Fixed in rG9e81249d7614

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76325