[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/LanguageExtensions.rst:511
+    *r = *a +  (*b * *c);
+  }
+
----------------
rjmccall wrote:
> 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.
I wasn't sure if that would be fine, but it indeed makes things much more readable. Updated.


================
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.
----------------
rjmccall wrote:
> 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`".
That's much better, thanks! I've also applied your suggestion to exclude `long double` for now.


================
Comment at: clang/docs/MatrixTypes.rst:167
+operations on matrix types match the behavior of the elementwise operations
+in the corresponding expansions provided above.
+
----------------
rjmccall wrote:
> 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`.
Updated, thanks!


================
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``.
+
----------------
rjmccall wrote:
> "omitted".
> 
> I would expect these operands to have type either `size_t` or `ptrdiff_t`.  Of course it only really matters for `columnStride`.
> "omitted".

Done, thanks!

> I would expect these operands to have type either size_t or ptrdiff_t. Of course it only really matters for columnStride.
Yes, I update them to size_t. This should give the implementations the most freedom with respect to choosing the implementation defined limits of rows/columns. `size_t` also makes the most sense for the stride I think, as it is required to be >= the number of rows in the matrix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612