[LangRef] Fix description of shape args for matrix.multiply.

[braedy]

Summary

Currently all code instances within the matrix lowering pass consider
matrix A to be MxN and B to be NxK, producing C which is MxK. Anyone
interacting with this API after reading the docs but without reading the pass
would expect A: MxK, B: KxN, and C: MxN. These changes bring the documentation
in line with the implementation.

One point of concern with this, the original signature as described in the docs
may be better or at least more expected. The interface as it was written
reflected other common matrix multiplication interfaces such as BLAS'[1], where
the matrices are MxK, KxN, MxN respectively. Choosing to honor this requires
changing code and tests instead, but should be mostly just renaming of variables.

[1] http://www.netlib.org/lapack/explore-html/db/dc9/group__single__blas__level3_gafe51bacb54592ff5de056acabd83c260.html#gafe51bacb54592ff5de056acabd83c260

[fhahn]

Thanks for raising this unfortunate inconsistency!

I think the interpretation used in the lowering is a bit more consistent with the interpretation of the shape arguments in the other intrinsics (which refer to the shape of the arguments directly), but the divergence from BLAS might indeed be a bit surprising. Perhaps using different names for M, N, K might be helpful to avoid confusion (e.g. something like NumRows1, NumCols1, NumCols2).

Changing the MatrixBuilder/LowerMatrixIntrinsics to use the different interpretation would indeed not be too much work in llvm-project, but it would break compatibility with modules constructed with the current MatrixBuilder. Given that code using the current interpretation in the langref would not work as expected anyways, I think we should try to preserve compatibility with working modules and update the LangRef. But it would be good to hear what other users think (all users of the intrinsics I am aware of should be on the review I think)

[braedy]

Theoretically, renaming the arguments from (M, N, K) to (M, K, N) doesn't break compatibility but updates the names to match BLAS, though the argument order no longer matches BLAS.

If you'd prefer to rename with completely different names, I'm not sure there's a good name involving columns or rows for the middle dimension. My suggestion would be outer1, outer2, inner.

[fhahn]

In D80663#2060263, @braedy wrote:

Theoretically, renaming the arguments from (M, N, K) to (M, K, N) doesn't break compatibility but updates the names to match BLAS, though the argument order no longer matches BLAS.

If you'd prefer to rename with completely different names, I'm not sure there's a good name involving columns or rows for the middle dimension. My suggestion would be outer1, outer2, inner.

I think we should go with renaming to different names, to hopefully reduce the risk of further confusion. Something like Outer1, Outer2, Inner seems good, perhaps even OuterRows, OuterColumns?

In any case, if there aren't any concerns with updating LangRef until Monday or so I think we should submit the update then :)

For the title of the revision, I would suggest updating to something more descriptive, like [LangRef] Fix description of shape args for matrix.multiply.

[braedy]

My concern using rows/columns in any of the argument names is that one of the dimensions changes from rows to columns, but you're right that perhaps outer isn't particularly descriptive enough and would require reading the overview instead of being able to understand it from just the signature.

[fhahn]

In D80663#2063539, @braedy wrote:

My concern using rows/columns in any of the argument names is that one of the dimensions changes from rows to columns, but you're right that perhaps outer isn't particularly descriptive enough and would require reading the overview instead of being able to understand it from just the signature.

Ah right. It doesn't change for the outer dimensions IIUC (so OuterRows & OuterColumns would be fine). it changes for the inner dimensions, which could be just Inner (or InnerDim)?

[braedy]

Ok, I'll make the changes.

[braedy]

Update names as per discussion.

[fhahn]

LGTM, thanks for addressing this issue!

[fhahn]

(please let me know if you need someone to commit this on your behalf)

[braedy]

I don't have commit access, so yes. Please and thank you!

[fhahn]

In D80663#2068922, @braedy wrote:

I don't have commit access, so yes. Please and thank you!

Great, I'll do so tomorrow, unless there are any additional comments.

llvm/docs/LangRef.rst

 
 ::
 
       declare vectorty @llvm.matrix.multiply.*(vectorty %A, vectorty %B, i32 <M>, i32 <N>, i32 <K>)
 
 Overview:
 """""""""
 
-The '``llvm.matrix.multiply.*``' intrinsic treats %A as matrix with <M> rows and <K> columns, %B as
-matrix with <K> rows and <N> columns and multiplies them. The result matrix is returned embedded in the
+The '``llvm.matrix.multiply.*``' intrinsic treats %A as matrix with <M> rows and <N> columns, %B as
+matrix with <N> rows and <K> columns and multiplies them. The result matrix is returned embedded in the
 result vector.
 
 Arguments:
 """"""""""
 
 The <M>, <N> and <K> arguments must be constant integers.  The vector argument %A
-must have <M> * <K> elements, %B must have <K> * <N> elements and the returned
-vector must have <M> * <N> elements.
+must have <M> * <N> elements, %B must have <N> * <K> elements and the returned
+vector must have <M> * <K> elements.
 
 
 '``llvm.matrix.columnwise.load.*``' Intrinsic
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Syntax:
 """""""