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