[PATCH] D60000: [llvm-exegesis] Post-processing for chained instrs in latency mode (PR41275)

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

Some early comments on the linear algebra library.
Let me sync with @courbet first.



================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:30
+
+template <typename T, typename Derived> struct MatrixInterface {
+  using value_type = T;
----------------
`static_assert` to make sure that `std::is_base_of<T, Derived>::value==true`


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:33
+
+  //  MatrixInterface() = delete;
+  MatrixInterface &operator=(MatrixInterface) = delete;
----------------
fix


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:37
+
+  int getNumRows() const { return ((const Derived *)this)->getNumRows(); }
+  int getNumColumns() const { return ((const Derived *)this)->getNumColumns(); }
----------------
gchatelet wrote:
> These could be properties `rows()` `columns()`.
you may want to provide a function to factor the cast operation in.


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:37-38
+
+  int getNumRows() const { return ((const Derived *)this)->getNumRows(); }
+  int getNumColumns() const { return ((const Derived *)this)->getNumColumns(); }
+
----------------
These could be properties `rows()` `columns()`.


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:60-64
+  //  Matrix(const Matrix &) = delete;
+  //  Matrix(Matrix &&) = delete;
+  //  Matrix &operator=(Matrix) = delete;
+  //  Matrix &operator=(const Matrix &) = delete;
+  //  Matrix &operator=(Matrix &&) = delete;
----------------
fix


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:66-67
+
+  int getNumRows() const { return num_rows; };
+  int getNumColumns() const { return num_cols; };
+
----------------
ditto - properties


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:82
+
+template <typename T, typename InnerTy>
+class TransposedMatrix
----------------
`MatrixTy`


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:83
+template <typename T, typename InnerTy>
+class TransposedMatrix
+    : public MatrixInterface<T, TransposedMatrix<T, InnerTy>> {
----------------
AFAIU from the implementation, this really is a `TransposedMatrixView`. I think it's important to highlight because `m` should outlive the `TransposedMatrix` object.


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:85
+    : public MatrixInterface<T, TransposedMatrix<T, InnerTy>> {
+  InnerTy &m;
+
----------------
`matrix`


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:85
+    : public MatrixInterface<T, TransposedMatrix<T, InnerTy>> {
+  InnerTy &m;
+
----------------
gchatelet wrote:
> `matrix`
Using a reference instead of a pointer prevents rebinding of this object (can't move, can't reassign). Is it a design decision?


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:97
+template <typename T, typename LHSTy, typename RHSTy>
+class AugmentedMatrix
+    : public MatrixInterface<T, AugmentedMatrix<T, LHSTy, RHSTy>> {
----------------
This is still a `View` AFAICT


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:140-141
+
+/// Get a square matrix with all elements being zero except the elements
+/// on the diagonal, which are ones.
+template <typename T, typename LHSTy>
----------------
Wrong comment


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:149
+template <typename T, typename LHSTy>
+bool isUpperTriangularMatrix(MatrixInterface<T, LHSTy> &LHS) {
+  for (int row = 0; row != LHS.getNumRows(); ++row) {
----------------
`const`


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:197
+    for (int col = 0; col != LHS.getNumColumns(); ++col) {
+      if (LHS(row, col) != (col == row) ? 1 : 0)
+        return false;
----------------
This is hard to read, do you mind introducing a variable for the expected value?


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:209
+template <typename T, typename LHSTy>
+int getLeadingCoeffientColumn(MatrixInterface<T, LHSTy> &LHS, int row) {
+  int col = 0;
----------------
`const`


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:220
+template <typename T, typename LHSTy>
+int isZeroRow(MatrixInterface<T, LHSTy> &LHS, int row) {
+  return getLeadingCoeffientColumn(LHS, row) == LHS.getNumColumns();
----------------
`const` here and below


================
Comment at: llvm/include/llvm/Support/LinearAlgebra.h:352
+template <typename T, typename LHSTy>
+Matrix<T> getInverseMatrix(MatrixInterface<T, LHSTy> &LHS) {
+  assert(isSquareMatrix(LHS));
----------------
`const` here and below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60000