The user guide states that the packages are designed for people familiar with matlab. I have never used matlab, so some of this feedback is based on Google searches. 1 - The index types are natural, but positive would be more like matlab. The guide claims this is a concession to C, but it seems illogical to mimic matlab everywhere except in the basic type definitions. On the other hand, it seems possible that synthesizers might implement 0-based index logic more efficiently, but that is not a concession to C. I suggest that the index convention should be "1 to n" to better match matlab. 2 - The guide states that objects declared with a downto range will be "reversed" by the operations. I think the meaning of "reversed" could be misunderstood. A person who thinks of the matrix visually, might expect that the input will be renumbered so that the "upper left" corner will be treated as index (0,0). A person who thinks mostly of the index values may expect the index (0,0) to be treated as the upper-left corner, regardless of the direction. Also, I think that this package does not follow the precedent set by prior VHDL packages regarding the significance of an array's direction vs its index values. For that matter, the LRM specifies that default array operations take inputs left-to-right with relative disregard for the index values. For example: constant uX : unsigned ( 3 downto 0 ) := "0001"; constant uY : unsigned ( 0 to 3 ) := "0001"; constant \uX+uY\ : unsigned := uX + uY; -- = "0010" constant \uX+uX\ : unsigned := uX + uX; -- = "0010" constant mX : real_matrix ( 0 to 1, 0 to 1 ) := (( 1.0, 0.0 ), ( 0.0, 1.0 )); constant mY : real_matrix ( 1 downto 0, 1 downto 0 ) := (( 1.0, 0.0 ), ( 0.0, 1.0 )); constant \mX+mY\ : real_matrix := mX + mY; -- (( 1.0, 1.0 ), -- ( 1.0, 1.0 )) constant \mX+mX\ : real_matrix := mX + mX; -- (( 2.0, 0.0 ), -- ( 0.0, 2.0 )) Because the meaning of "reversed" can be misunderstood, and because this package emphasizes index values over index direction (in contrast to the LRM and other standard packages), I suggest that these operations should throw an error if the inputs fail to comply with the "1 to n" convention. 3 - The submatrix function mimics some matlab indexing options. In matlab, the (i:j) pattern, returns the "slice" starting at index i and ending at index j. The VHDL submatrix function accepts a starting row/column and a number of rows/columns. I suggest mimicking matlab more closely when possible. 4 - In function "*"(matrix, matrix), I think this implementation is equivalent and simpler (a similar transform applies to "*"(vector, matrix)): for i in result'range(1) loop for j in result'range(2) loop result (i, j) := 0.0; --result (i, j) := l(i+l'low(1), l'low(2)) * r(r'low(1), j+r'low(2)); for k in 0 to l'length(2)-1 loop result (i, j) := result (i, j) + (l(i+l'low(1), k+l'low(2)) * r(k+r'low(1), j+r'low(2))); end loop; -- k end loop; -- j end loop; -- i 5 - As it is written, calling "/"(matrix, matrix) with malformed inputs will produce a confusing message about the requirements of matrix multiplication. 6 - "/"(matrix, matrix) is written by calling l * inv(r). I'm surprised that "-"(matrix, matrix) is not written by calling l + ( -r ). 7 - Somebody who knows what every computer scientist should know about floating-point arithmetic should review the implementation for "/"(matrix, real). I'm not sure that ((1.0/R) * L) is always equal to (R/L). 8 - According to the comment in real_matrix_pkg_body.vhd, the purpose of the function "ones" is to return a matrix of zeros -- a copy-paste error, I'm sure. 9 - The recursion for "**"(matrix, integer) will return ones when pow=0. It seems likely that should be "eye". 10 - The recursion for "**" seems too complex. I think the termination for pow=2 could be deleted (assuming the pow=0 should return eye). 11 - The exclude function has a purpose comment for the SubMatrix function. 12 - The SubMatrix function has parameters x, y where the exclude function has parameters row, column serving the same purpose. 13 - The Submatrix has a comment that refers to "position l,r" but the function has no parameters named l and r. 14 - The BuildMatrix has a comment that refers to "position l, r" but has no parameters with those names. 15 - BuildMatrix has commented out the error report statements. 16 - Many functions have some input parameters that are explicitly constant while other parameters are implicitly constant. Why? 17 - Many comments do not correspond to the adjacent code. 18 - rot90(arg, dim) seems to correspond to matlab's rot90(A, k). Your implementation does nothing if dim is outside the range -3 to 3. It seems likely you want to assert that dim is in your supported range or you want to modulo dim into +/- 3. Paul Paul.Butler@ni.com | (512) 683-8743 | National Instruments | Austin, TX From: Jim Lewis <jim@synthworks.com> To: "vhdl-200x@eda.org" <vhdl-200x@eda.org>, Date: 06/22/2014 01:31 PM Subject: [vhdl-200x] Please Review Matrix Math Package Sent by: owner-vhdl-200x@eda.org Hi, Please review the proposed matrix math package. It is proposed to become part of IEEE 1076 and is a necessary part for the 1076.1 VHDL-AMS standard. You can find the user guide here: http://www.vhdl.org/fphdl/real_matrix_ug.pdf You can find the packages here: http://www.vhdl.org/fphdl/real_matrix_pkg.zip Speak now and be heard - otherwise don't complain later. Best Regards, Jim -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Jim Lewis Jim@SynthWorks.com VHDL Training Expert http://www.SynthWorks.com IEEE VHDL Working Group Chair OSVVM, Chief Architect and Cofounder 1-503-320-0782 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean. -- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.Received on Mon Jun 23 15:10:09 2014
This archive was generated by hypermail 2.1.8 : Mon Jun 23 2014 - 15:10:42 PDT