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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
