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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Scanned through the first bit.



================
Comment at: clang/docs/LanguageExtensions.rst:500
+Clang provides a matrix extension, which is currently being implemented. See
+:ref:`matrixsupport` for more details.
+
----------------
This should include just a bit more detail about the extension.  I would suggest:

> Clang supports matrix types as an experimental extension.  See :`ref`matrices` for more details.


================
Comment at: clang/docs/MatrixSupport.rst:3
+Matrix Support
+==================
+
----------------
This extension should be called something like "Matrices" or "Matrix Types".  The "X Support" name makes it sound like it's a support layer for some external technology.


================
Comment at: clang/docs/MatrixSupport.rst:12
+matrix math on the C/C++ level. The draft specification can be found
+:ref:`below <matrixsupport-draftspec>`.
+
----------------
1. "Clang provides a C/C++ language extension that allows users to directly express fixed-size matrices as language values and perform arithmetic on them."

2. This document *is* the specification, there's nothing to cross-reference it.


================
Comment at: clang/docs/MatrixSupport.rst:14
+
+Note that the implementation is currently in progress.
+
----------------
"This feature is currently experimental, and both its design and its implementation are in flux."


================
Comment at: clang/docs/MatrixSupport.rst:30
+directly. An *attribute-argument-clause* must be present and it shall have the
+form:
+
----------------
You can assume the existence of a hypothetical external language specification for GNU attribute syntax, so these starting paragraphs whittle down to:

> Matrix types can be declared by adding the `matrix_type` attribute to the declaration of a `typedef` (or a C++ alias declaration).  The underlying type of the `typedef` must be an unqualified integer or floating-point type.  The attribute takes two arguments, both of which must be integer constant expressions that evaluate to a value greater than zero.  The first specifies the number of rows, and the second specifies the number of columns.  The underlying type of the `typedef` becomes a matrix type with the given dimensions and an element type of the former underlying type.

The paragraph about redeclarations is good.


================
Comment at: clang/docs/MatrixSupport.rst:48
+Matrix Type
+-----------
+
----------------
I would put this first before getting into the spelling.  You can also put the stuff about implementation limits on dimensions in here.


================
Comment at: clang/docs/MatrixSupport.rst:55
+padding in a way compatible with an array of at least that many elements of the
+underlying *element type*.
+
----------------
Do you actually want to guarantee this layout in the language specification?  I would just say that a matrix includes storage for `rows * columns` elements but that the interior layout and overall size and alignment are implementation-defined.


================
Comment at: clang/docs/MatrixSupport.rst:57
+
+A matrix type is a *scalar type* and its alignment is implementation defined.
+
----------------
These are both important to include, but they're unrelated and shouldn't be in the same sentence.


================
Comment at: clang/docs/MatrixSupport.rst:64
+
+TODO: Specify how matrix values are passed to functions.
+
----------------
That doesn't belong in a language specification, but you could reasonably add a non-normative section at the end about the decisions that Clang currently makes for things like size, alignment, internal layout, and argument/result conventions.


================
Comment at: clang/docs/MatrixSupport.rst:106
+expression. ``expression`` shall not be a comma expression, and shall be a
+prvalue of unscoped enumeration or integral type. Given the expression
+``E1[E2][E3]`` the result is an lvalue of the same type as the underlying
----------------
It would be better to say explicitly that "The index expressions shall..."


================
Comment at: clang/docs/MatrixSupport.rst:110
+the matrix. The expression E1 is sequenced before E2 and E3. The expressions
+E2 and E3 are unsequenced.
+
----------------
I'd put all this like:

> An expression of the form ``E1 [E2] [E3]``, where ``E1`` has matrix type ``cv M``, is a matrix element access expression.  Let ``T`` be the element type of ``M``, and let ``R`` and ``C`` be the number of rows and columns in ``M`` respectively.  The index expressions shall have integral or unscoped enumeration type and shall not be uses of the comma operator unless parenthesized.  The first index expression shall evaluate to a non-negative value less than ``R``, and the second index expression shall evaluate to a non-negative value less than ``C``, or else the expression has undefined behavior.  If ``E1`` is a prvalue, the result is a prvalue with type ``T`` and is the value of the element at the given row and column in the matrix.  Otherwise, the result is a glvalue with type ``cv T`` and with the same value category as ``E1`` which refers to the element at the given row and column in the matrix.


================
Comment at: clang/docs/MatrixSupport.rst:118
+builtins to extract rows and columns from a matrix. This makes the operations
+more explicit.
+
----------------
You should add a normative paragraph saying that a program is ill-formed if it insufficiently subscripts into a matrix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612