[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[John McCall via Phabricator via cfe-commits]

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