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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Reading through the rest of the spec.



================
Comment at: clang/docs/LanguageExtensions.rst:500
+Clang provides a matrix extension, which is currently being implemented. See
+:ref:`matrixsupport` for more details.
+
----------------
rjmccall wrote:
> This should include just a bit more detail about the extension.  I would suggest:
> 
> > Clang supports matrix types as an experimental extension.  See :`ref`matrices` for more details.
If we're calling the extension "matrix types", that should be reflected in this section name and in the file name.


================
Comment at: clang/docs/MatrixSupport.rst:28
+includes storage for ``rows * columns`` values of the *element type*. The
+internal layout, overall size and alignment are implementation-defined.
+
----------------
No need to italicize "element type" the second time.

The italics introduce a term, so consider italicizing "rows" and "columns" as well in the first sentence.


================
Comment at: clang/docs/MatrixSupport.rst:39
+
+Future Work: Initialization syntax.
+
----------------
Maybe break the TODOs here into their own sections, which would come much later.


================
Comment at: clang/docs/MatrixSupport.rst:70
+between the original and resulting type, the program is ill-formed. Otherwise
+the resulting value is as follows:
+
----------------
I don't think you need to list out the kinds of promotion and conversion here, and it doesn't make sense to define the "resulting type" this way when it's really a parameter.  I'd just say:

> A value of matrix type can be converted to another matrix type if the number of rows and columns are the size and the value's elements can be converted to the element type of the result type.  The result is a matrix where each element is the converted corresponding element.

> A value of non-matrix type can be converted to a matrix type if it can be converted to the element type of the matrix.  The result is a matrix where all elements are the converted original value.


================
Comment at: clang/docs/MatrixSupport.rst:126
+The operands of ``+``, ``-`` and ``*`` shall have either matrix type, arithmetic or
+unscoped enumeration type. At least one of the operands shall be of matrix type.
+
----------------
I don't think this paragraph adds anything, and the restriction is kindof weird — it's just a restriction on when to consider applying these rules, rather than a restriction with absolute significance.

Also, "arithmetic type" includes unscoped enumeration types in both C and C++.


================
Comment at: clang/docs/MatrixSupport.rst:129
+For ``BIN_OP`` in ``+``, ``-``, ``*`` given the expression ``M1 BIN_OP M2`` where, for
+``*``, one of M1 or M2 is of arithmetic type:
+
----------------
Here I think you can say "where at least one of M1 or M2 is of matrix type and, for `*`, the other is of arithmetic type".

I think you'll need to separately describe the restrictions on `+=`, `-=`, and `*=`, but you should be able to say that the semantics are as if for the expansion.


================
Comment at: clang/docs/MatrixSupport.rst:176
+
+Matrix Type builtin Operations
+------------------------------
----------------
"builtin" should be capitalized here.


================
Comment at: clang/docs/MatrixSupport.rst:211
+
+``M __builtin_matrix_column_load(T *ptr, int row, int col, int stride)``
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612