[PATCH] D83477: [Matrix] Tighten LangRef definitions and Verifier checks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! Some optional nits related to wording inline (I think it would be good to start the sentences for the arguments with a `The`).



================
Comment at: llvm/docs/LangRef.rst:15502
+matrixes are passed and returned as vectors. This means that for a ``R`` x
+``C`` matrix, element ``i`` of column ``j`` is at index ``j * R + i`` in its
+vector, with indices starting at 0. Currently column-major layout is assumed.
----------------
maybe something like `in the corresponding vector` instead of `in its vector`, where it might be a little unclear what `its` refers to.


================
Comment at: llvm/docs/LangRef.rst:15527
 
-The <Rows> and <Cols> arguments must be constant integers. The vector argument
-%In and the returned vector must have <Rows> * <Cols> elements.
+First argument %In is vector that corresponds to a <Rows> x <Cols> matrix.
+Thus, arguments <Rows> and <Cols> correspond to the number of rows and columns,
----------------
`The first` ..?


================
Comment at: llvm/docs/LangRef.rst:15554
 
-The <OuterRows>, <Inner> and <OuterColumns> arguments must be constant
-integers. The vector argument %A must have <OuterRows> * <Inner> elements, %B
-must have <Inner> * <OuterColumns> elements and the returned vector must have
-<OuterRows> * <OuterColumns> elements.
+First vector argument %A corresponds to a matrix with <OuterRows> * <Inner>
+elements, and second argument %B to a matrix with <Inner> * <OuterColumns>
----------------
`The first`... `and the second` ...?


================
Comment at: llvm/docs/LangRef.rst:15558
+constant integers. The returned vector must have <OuterRows> * <OuterColumns>
+elements. Vectors %A, %B, and the returned vector all have the same float or
+integer element type.
----------------
`must all have` ?


================
Comment at: llvm/docs/LangRef.rst:15588
 
-The <IsVolatile>, <Rows> and <Cols> arguments must be constant integers. The
-returned vector must have <Rows> * <Cols> elements. %Stride must be >= <Rows>.
+First argument %Ptr is a pointer type to the returned vector type, and
+correponds to the start address to load from. Second argument %Stride is a
----------------
`The first...`?


================
Comment at: llvm/docs/LangRef.rst:15589
+First argument %Ptr is a pointer type to the returned vector type, and
+correponds to the start address to load from. Second argument %Stride is a
+postive, constant integer with %Stride ``>=`` <Rows>. %Stride is used to compute
----------------
`The second`?


================
Comment at: llvm/docs/LangRef.rst:15592
+the column memory addresses. I.e., for a column ``C``, its start memory
+addresses is calculated with %Ptr + ``C`` * %Stride. Third Argument
+<IsVolatile> is a boolean value.  The fourth and fifth arguments, <Rows> and
----------------
`The third`


================
Comment at: llvm/docs/LangRef.rst:15628
 
-The <IsVolatile>, <Rows>, <Cols> arguments must be constant integers. The
-vector argument %In must have <Rows> * <Cols> elements. %Stride must be >= <Rows>.
+First argument %In is vector that corresponds to a <Rows> x <Cols> matrix to be
+stored to memory. Second argument %Ptr is a pointer type to the vector type of
----------------
`The first argument %In is a vector`?


================
Comment at: llvm/docs/LangRef.rst:15629
+First argument %In is vector that corresponds to a <Rows> x <Cols> matrix to be
+stored to memory. Second argument %Ptr is a pointer type to the vector type of
+%In, and is the start address of the matrix in memory.  Third argument %Stride
----------------
`The second argument %Ptr is a pointer to the`?


================
Comment at: llvm/docs/LangRef.rst:15630
+stored to memory. Second argument %Ptr is a pointer type to the vector type of
+%In, and is the start address of the matrix in memory.  Third argument %Stride
+is a positive, constant integer with %Stride ``>=`` <Rows>. %Stride is used to
----------------
`The third`?


================
Comment at: llvm/docs/LangRef.rst:15633
+compute the column memory addresses. I.e., for a column ``C``, its start memory
+addresses is calculated with %Ptr + ``C`` * %Stride. Fourth argument
+<IsVolatile> is a boolean value. Arguments <Rows> and <Cols> correspond to the
----------------
`The fourth`?


================
Comment at: llvm/docs/LangRef.rst:15634
+addresses is calculated with %Ptr + ``C`` * %Stride. Fourth argument
+<IsVolatile> is a boolean value. Arguments <Rows> and <Cols> correspond to the
+number of rows and columns, respectively, and must be positive, constant
----------------
`The arguments`?


================
Comment at: llvm/lib/IR/Verifier.cpp:5069
+      Assert(ResultTy->getElementType() == Op1ElemTy,
+             "Type mismatch of the result and second operand vector!", IF);
+
----------------
It would be good to be consistent with the capitalization/puncation with the existing message at 5073 or update the message there.

Also, it might be good to include `vector element type` in the message, as in the message for Op0.


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

https://reviews.llvm.org/D83477