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

[Florian Hahn via Phabricator via cfe-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
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:
> > > 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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612