Page MenuHomePhabricator

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

Authored by braedy on May 27 2020, 2:12 PM.

Details

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

Diff Detail

Event Timeline

braedy created this revision.May 27 2020, 2:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 2:12 PM
fhahn added a comment.May 28 2020, 5:36 AM

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 added a comment.EditedMay 28 2020, 8:26 AM

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 added a comment.May 29 2020, 7:45 AM

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 retitled this revision from Change description to match code representation. to [LangRef] Fix description of shape args for matrix.multiply..May 29 2020, 7:59 AM

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 added a comment.Jun 1 2020, 12:11 PM

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 added a comment.Jun 1 2020, 1:17 PM

Ok, I'll make the changes.

braedy updated this revision to Diff 267760.Jun 1 2020, 4:21 PM

Update names as per discussion.

fhahn accepted this revision.Jun 2 2020, 2:31 AM

LGTM, thanks for addressing this issue!

This revision is now accepted and ready to land.Jun 2 2020, 2:31 AM
fhahn added a comment.Jun 2 2020, 2:32 AM

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

braedy added a comment.Jun 2 2020, 8:38 AM

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

fhahn added a comment.Jun 2 2020, 11:15 AM

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

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

This revision was automatically updated to reflect the committed changes.