[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/LanguageExtensions.rst:511
+    *r = *a +  (*b * *c);
+  }
+
----------------
This is kindof an unnecessarily unreadable example.  I know you haven't decided on calling convention treatment yet, but maybe the leading example could be just a little ahead of the implementation and just take the matrices as arguments and then return the result.


================
Comment at: clang/docs/MatrixTypes.rst:29
+A *matrix element type* must be a real type (as in C99 6.2.5p17) excluding
+enumeration types or an implementation-defined half-precision floating point
+type, otherwise the program is ill-formed.
----------------
fhahn wrote:
> rjmccall wrote:
> > fhahn wrote:
> > > SjoerdMeijer wrote:
> > > > Now I am wondering if this requires some explanations on binary operations for these implemenation-defined types? For example, for `__fp16` and matrices with this `__fp16` element type, I assume arithmetic is performed in at least the (single) floating-point precision. So I guess in section "Arithmetic Conversions" a rule needs to be added that the conversion of these implementation defined types need to performed?
> > > > 
> > > I don't think we need to specifically discuss the implementation defined types here, as the conversions and binary operator definitions are framed in terms of the existing rules for the element types used. I am potentially missing something, but with the current wording the conversions for `__fp16` would use the conversion rules for that type and the binary operators would use the arithmetic rules for it.
> > Yeah, for the scalar conversions / scalar operands, you should just say that the source has to be a real type and not otherwise restrict it.  All of those types should already be convertible to any matrix element type.
> Thanks, I've updated the wording to ensure the scalar values are of a real type in the scalar -> matrix conversion and scalar, matrix binary operator contexts. I hope that is enough to clarify things.
This would be clearer as something like:

> Currently, the element type of a matrix is only permitted to be one of the following types:
>
> - an integer type (as in C2x 6.2.5p19), but excluding enumerated types and `_Bool`
> - a standard floating type (as in C2x 6.2.5p10)
> - a half-precision floating point type, if one is supported on the target
>
> Other types may be supported in the future.

Although I don't know if you actually want to unconditionally support `long double`; you might just want to say "the standard floating types `float` and `double`".


================
Comment at: clang/docs/MatrixTypes.rst:62
+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
----------------
s/size/same/


================
Comment at: clang/docs/MatrixTypes.rst:66
+
+A value of a real 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
----------------
"A value of any real type (as in C2x 6.2.5p17) can be converted..."


================
Comment at: clang/docs/MatrixTypes.rst:167
+operations on matrix types match the behavior of the elementwise operations
+in the corresponding expansions provided above.
+
----------------
The expansions have a lot of statement boundaries that contraction wouldn't be allowed across.  I'd suggest saying something like:

> Operations on floating-point matrices have the same rounding and floating-point environment behavior as ordinary floating-point operations in the expression's context.  For the purposes of floating-point contraction, all calculations done as part of a matrix operation are considered intermediate operations, and their results need not be rounded to the format of the element type until the final result in the containing expression.  This is subject to the normal restrictions on contraction, such as `#pragma STDC FP_CONTRACT`.


================
Comment at: clang/docs/MatrixTypes.rst:216
+to ``row`` and ``col`` respectively. The parameter ``columnStride`` is optional
+and if ommitted ``row`` is used as ``columnStride``.
+
----------------
"omitted".

I would expect these operands to have type either `size_t` or `ptrdiff_t`.  Of course it only really matters for `columnStride`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612