rjmccall added inline comments.
================
Comment at: clang/docs/MatrixSupport.rst:211
+
+``M __builtin_matrix_column_load(T *ptr, int row, int col, int stride)``
+
----------------
fhahn wrote:
> rjmccall wrote:
> > This name sounds like it's loading a column, when I think you're saying
> > that the memory has to be in column-major order.
> >
> > I would call `stride` something like `columnStride` to make it clear that
> > it's the stride between columns, as opposed to a stride between the
> > elements within a column, which is also something that's theoretically
> > interesting.
> >
> > Should `stride` be an optional argument to make it easier to write the (I
> > expect) common case where the matrix is dense?
> > This name sounds like it's loading a column, when I think you're saying
> > that the memory has to be in column-major order.
>
> Yes that is correct. Maybe __builtin_matrix_columnwise_load would be slightly
> better?
>
> > Should stride be an optional argument to make it easier to write the (I
> > expect) common case where the matrix is dense?
> Yes that would be very convenient, especially now that casting between
> element wise pointers and matrixes is not allowed. I've added a sentence to
> the remarks for both the load and store builtins.
> Yes that is correct. Maybe __builtin_matrix_columnwise_load would be slightly
> better?
The term of art is "column-major"; I don't think avoiding an "extra word" is a
good enough reason to invent something else.
`__builtin_matrix_column_major_load` sounds fine to me.
================
Comment at: clang/docs/MatrixTypes.rst:79
+ floating point type, convert the integer or floating point operand to the
+ underlying element type of the operand of matrix type.
+
----------------
You should standardize on one term and then be clear what you mean by it. Here
you're saying "integer or floating point type", but elsewhere you use
"arithmetic type". Unfortunately, the standard terms mean somewhat different
things in different standards: "integer" includes enums in C but not in C++,
"arithmetic" doesn't include complex types in C++ (although it does by
extension in Clang), etc. I think for operands you probably want arithmetic
types in the real domain (which in Clang is `isRealType()`). However, you'll
want to use a narrower term for the restriction on element types because Clang
does support fixed-point types, but you probably don't want to support matrices
of them quite yet (and you may not want to allow matrices of bools, either).
Also, your description of the scalar conversions no longer promotes them to
matrix type.
================
Comment at: clang/docs/MatrixTypes.rst:123
+ M2 are of arithmetic type, they are broadcast to matrices here. — end note ]
+* The matrix types of M1 and M2 shall have the same number of rows and columns.
+* The result is equivalent to Res in the following where col is the number of
----------------
They also have to have the same element types, right? So they have to be the
same types?
================
Comment at: clang/docs/MatrixTypes.rst:138
+* The type of ``M1`` shall have the same number of columns as the type of
``M2`` has
+ rows.
+* The resulting type, ``MTy``, is the result of applying the usual arithmetic
----------------
Same point about element types.
================
Comment at: clang/docs/MatrixTypes.rst:141
+ conversions to ``M1`` and ``M2``, but with the same number of rows as M1’s
matrix
+ type and the same number of columns as M2’s matrix type.
+* The result is equivalent to ``Res`` in the following where ``EltTy`` is the
----------------
The easier way to put this now is that it's a matrix type whose element type is
the common element type, but with the number of rows of `M1` and the number of
columns of `M2`.
================
Comment at: clang/docs/MatrixTypes.rst:152
+ EltTy Elt = 0;
+ for (int K = 0; K < inner; ++K) {
+ Elt += M1[R][K] * M2[K][C];
----------------
`inner` is not defined.
================
Comment at: clang/docs/MatrixTypes.rst:164
+Clang option to override this behavior and allow contraction of those
+operations (e.g. *-ffp-contract=matrix*).
+
----------------
This is about rounding, not rounding "errors".
The definition of matrix multiply you've written it above would actually permit
an FMA under C's default rules.
More broadly, I think you need to define how the FP contraction and environment
rules affect matrix arithmetic expressions. If FP contraction is enabled, can
`S * M1 + M2` perform elementwise FMAs?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76612/new/
https://reviews.llvm.org/D76612
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
