[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

Wed Apr 15 09:16:57 PDT 2020

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:
> fhahn wrote:
> > 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.`
> > 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?
> 
> C says:
> 
> > The integer and real floating types are collectively called *real types*.
> 
> > The type `char`, the signed and unsigned integer types, and the enumerated types are collectively called *integer types*.
> 
> > The standard and extended unsigned integer types are collectively called *unsigned integer types*.
> 
> > The type `_Bool` and the unsigned integer types that correspond to the standard signed integer types are the *standard unsigned integer types*.
> 
> Embedded C (TR 18037) says:
> 
> > Clause 6.2.5 - Types, paragraph 17: change last sentence as follows.
> >
> > Integer, fixed-point and real floating types are collectively called *real types*.
> 
> So you'll have to explicitly exclude enumerated types, `_Bool`, and the fixed-point types.
Ah thanks, I missed `TR 18037`. Sorry about that! I've updated the wording as suggested.  

================
Comment at: clang/docs/MatrixTypes.rst:27
+internal layout, overall size and alignment are implementation-defined.
+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
----------------
SjoerdMeijer wrote:
> fhahn wrote:
> > SjoerdMeijer wrote:
> > > fhahn wrote:
> > > > SjoerdMeijer wrote:
> > > > > above you're using *element type* and here *matrix element type*. Since hopefully we're talking about the same things, "matrix *element type*" would be more consistent.
> > > > > 
> > > > > But this is just a nit, my main question is about the types:
> > > > > why not e.g. define this to be the C11 types, that include _FloatN types, so that we can include N=16? Or is this intentionally omitted? I haven't even checked if this is supported in the architecture extension, but might make sense? And also, an element type cannot be an integer type? 
> > > > > 
> > > > >  
> > > > > above you're using *element type* and here *matrix element type*. Since hopefully we're talking about the same things, "matrix *element type*" would be more consistent.
> > > > 
> > > > Yes it is referring to the same thing. I had a look at most uses, and in most cases `element type` is used to refer to the element type of a given matrix type. In that context it seems a bit verbose to use `matrix element type`, although I am more than happy to change that if it helps with clarifying things.
> > > > 
> > > > I intentionally used `matrix element type` in `Arithmetic Conversions`, because there it is standing on its own and refers exactly to the set of types defined as valid matrix element types here.
> > > > 
> > > > > why not e.g. define this to be the C11 types, that include _FloatN types, so that we can include N=16? Or is this intentionally omitted? I haven't even checked if this is supported in the architecture extension, but might make sense?
> > > > 
> > > > I couldn't find any reference to _FloatN types in the C11 draft version I checked. Do you by any chance have a reference to the _FloatN types?
> > > > 
> > > > > And also, an element type cannot be an integer type?
> > > > 
> > > > The current definition should include it (real types include integer and real floating point types according to  C99 6.2.5p17). I don't think there is any reason to exclude them I think.
> > > >>    why not e.g. define this to be the C11 types, that include _FloatN types, so that we can include N=16? Or is this intentionally omitted? I haven't even checked if this is supported in the architecture extension, but might make sense?
> > > >>
> > > > I couldn't find any reference to _FloatN types in the C11 draft version I checked. Do you by any chance have a reference to the _FloatN types?
> > > 
> > > Sorry, I was a bit imprecise here, it's an extension of C11: ISO/IEC TS 18661-3:2015.
> > > My thinking was it would be cool to support the "proper" half-precision type. I thought about this, because of "or an implementation-defined half-precision" mentioned just below here, of which probably __fp16 is an example.  If you refer to the C99 types, you probably don't even need to mention this (although it won't do any harm)?
> > > 
> > > >>  And also, an element type cannot be an integer type?
> > > >
> > > > The current definition should include it (real types include integer and real floating point types according to C99 6.2.5p17). I don't think there is any reason to exclude them I think.
> > > 
> > > Ok, cheers, wrote this from memory (forgot this), and didn't check the standard.
> > > Sorry, I was a bit imprecise here, it's an extension of C11: ISO/IEC TS 18661-3:2015.
> > > My thinking was it would be cool to support the "proper" half-precision type. I thought about this, because of "or an implementation-defined half-precision" mentioned just below here, of which probably __fp16 is an example. If you refer to the C99 types, you probably don't even need to mention this (although it won't do any harm)?
> > 
> > I am not sure what the exact wording should be, but the intention is to include both __fp16 and _Float16. I was hoping that would be covered as is, but I would be happy to clarify (unfortunately it is not entirely clear to me how to best word this)
> Ah, okay, I got it. How about a simple enumeration, e.g.:  
> 
>   A *matrix element type* must be a C99 real type, excluding enumeration types, the C11 ISO/IEC TS 18661 _Float16 type, the ARM ACLE __fp16 type, or an implementation-defined half-precision floating point type, otherwise the program is ill-formed.
Given that there are a few different half-precision floating point types with various levels of support in different compilers, I would prefer not to explicitly list them at the moment, while making it clear that they can also be supported. AFAIK there's work in progress to add Bfloat support to clang and I think we also would want to support that type in the future.

================
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.
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612