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

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/docs/MatrixSupport.rst:39
+
+Future Work: Initialization syntax.
+
----------------
rjmccall wrote:
> Maybe break the TODOs here into their own sections, which would come much later.
Done, I've moved the TODOs to a TODO section just after the builtins section.


================
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:
+
----------------
rjmccall wrote:
> 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.
I've kept the first paragraph (including the exclusion of assignment) and replaced the second with your suggestion.


================
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:
+
----------------
rjmccall wrote:
> 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.
I added the following at the end of the section

> For the ``+=``, ``-=`` and ``*=`` operators the semantics match their expanded variants.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612