[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/MatrixTypes.rst:79
+  floating point type, convert the integer or floating point operand to the
+  underlying element type of the operand of matrix type.
+
----------------
rjmccall wrote:
> You should standardize on one term and then be clear what you mean by it.  Here you're saying "integer or floating point type", but elsewhere you use "arithmetic type".  Unfortunately, the standard terms mean somewhat different things in different standards: "integer" includes enums in C but not in C++, "arithmetic" doesn't include complex types in C++ (although it does by extension in Clang), etc.  I think for operands you probably want arithmetic types in the real domain (which in Clang is `isRealType()`).  However, you'll want to use a narrower term for the restriction on element types because Clang does support fixed-point types, but you probably don't want to support matrices of them quite yet (and you may not want to allow matrices of bools, either).
> 
> Also, your description of the scalar conversions no longer promotes them to matrix type.
> You should standardize on one term and then be clear what you mean by it. Here you're saying "integer or floating point type", but elsewhere you use "arithmetic type". Unfortunately, the standard terms mean somewhat different things in different standards: "integer" includes enums in C but not in C++, "arithmetic" doesn't include complex types in C++ (although it does by extension in Clang), etc. I think for operands you probably want arithmetic types in the real domain (which in Clang is isRealType()). However, you'll want to use a narrower term for the restriction on element types because Clang does support fixed-point types, but you probably don't want to support matrices of them quite yet (and you may not want to allow matrices of bools, either).

I've added the following to the Matrix Type section:
` 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.`

Other places are updated to use `a valid matrix element type` instead.

I think we explicitly want to allow half-precision types (like __fp16 and Float16 in Clang). I think by referring to real type as in the C99 spec, we naturally exclude Clang's fixed-point types and bool, right? 

> Also, your description of the scalar conversions no longer promotes them to matrix type.

Right, I think we can just refer to the standard conversion rules here, as in

`If one operand is of matrix type and the other operand is of a valid matrix element type, convert the non-matrix type operand to the matrix type according to the standard conversion rules.`


================
Comment at: clang/docs/MatrixTypes.rst:123
+  M2 are of arithmetic type, they are broadcast to matrices here. — end note ]
+* The matrix types of M1 and M2 shall have the same number of rows and columns.
+* The result is equivalent to Res in the following where col is the number of
----------------
rjmccall wrote:
> They also have to have the same element types, right?  So they have to be the same types?
Yes, for 2 operands of a matrix type, they should be the same types now. Changed to  `M1 and M2 shall be of the same matrix type.`


================
Comment at: clang/docs/MatrixTypes.rst:138
+* The type of ``M1`` shall have the same number of columns as the type of ``M2`` has
+  rows.
+* The resulting type, ``MTy``, is the result of applying the usual arithmetic
----------------
rjmccall wrote:
> Same point about element types.
Added 
>The element types of ``M1`` and ``M2`` shall be the same type


================
Comment at: clang/docs/MatrixTypes.rst:141
+  conversions to ``M1`` and ``M2``, but with the same number of rows as M1’s matrix
+  type and the same number of columns as M2’s matrix type.
+* The result is equivalent to ``Res`` in the following where ``EltTy`` is the
----------------
rjmccall wrote:
> The easier way to put this now is that it's a matrix type whose element type is the common element type, but with the number of rows of `M1` and the number of columns of `M2`.
Replaced with 

> The resulting type, `MTy`, is a matrix type with the common element type, the number of rows of `M1` and the number of columns of `M2`.



================
Comment at: clang/docs/MatrixTypes.rst:152
+      EltTy Elt = 0;
+      for (int K = 0; K < inner; ++K) {
+        Elt += M1[R][K] * M2[K][C];
----------------
rjmccall wrote:
> `inner` is not defined.
Should be something like ` and ``inner`` is the number of columns of ``M1```


================
Comment at: clang/docs/MatrixTypes.rst:164
+Clang option to override this behavior and allow contraction of those
+operations (e.g. *-ffp-contract=matrix*).
+
----------------
rjmccall wrote:
> This is about rounding, not rounding "errors".
> 
> The definition of matrix multiply you've written it above would actually permit an FMA under C's default rules.
> 
> More broadly, I think you need to define how the FP contraction and environment rules affect matrix arithmetic expressions.  If FP contraction is enabled, can `S * M1 + M2` perform elementwise FMAs?
> This is about rounding, not rounding "errors".

Fixed.

> The definition of matrix multiply you've written it above would actually permit an FMA under C's default rules.

The goal of the wording it to match the existing behavior for the expanded version for compatibility reasons.I think Clang currently does not emit FMAs without contraction explicitly enabled, but GCC emits FMAs without contraction explicitly enabled for something like ` A * B + C`. I think the current wording allows for both, following the reasoning for both Clang's and GCC's current  behavior for the single element case.

> More broadly, I think you need to define how the FP contraction and environment rules affect matrix arithmetic expressions. If FP contraction is enabled, can S * M1 + M2 perform elementwise FMAs?

FP contraction and environment rules should match the corresponding expansions. So with fp-contraction enabled, `S * M1 + M2`. I re-worded the paragraph and hopefully it is clearer now:

> With respect to floating-point contraction, rounding and environment rules, operations on matrix types match the behavior of the elementwise operations in the corresponding expansions provided above.


I've moved the part of the clang option to the `Decision for the Implementation in Clang` section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612