[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

Sun May 31 16:13:55 PDT 2020

Kayjukh requested changes to this revision.
Kayjukh added a comment.
This revision now requires changes to proceed.

I added quite a lot of inline comments. To avoid redundancies for some of them, here are more general ones that apply to all the files:

- Don't use Doxygen tags in your comments.
- Use `///` for top-level comments (e.g. outside of functions).
- Prefer `SmallVector` over `std::vector` since the former can potentially be more efficient for small sizes.
- `Fraction` and `Matrix` look like they are only ever used with `int64_t` elements. Is there a need for them to be template classes?

================
Comment at: mlir/include/mlir/Analysis/AffineStructures.h:142

+  // \returns True if the set of constraints has no integer solution, and False
+  // if it does have a solution.
----------------
This looks like a Doxygen leftover.

================
Comment at: mlir/include/mlir/Analysis/AffineStructures.h:150
+  // Returns such a point if one exists, or an empty llvm::Optional otherwise.
+  llvm::Optional<std::vector<int64_t>> findIntegerSample() const;
+
----------------
`std::vector` is certainly not the right data structure to represent a point.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Fraction.h:1
+//===- Simplex.h - MLIR Fraction Class --------------------------*- C++ -*-===//
+//
----------------
`Simplex.h` -> `Fraction.h`

================
Comment at: mlir/include/mlir/Analysis/Presburger/Fraction.h:33
+int compare(const Fraction<T> &x, const Fraction<T> &y) {
+  auto diff = x.num * y.den - y.num * x.den;
+  if (diff > 0)
----------------
These kinds of operations may be dangerous in case someone would instantiate a `Fraction` with a small element type (e.g. `Fraction<int8_t>`). I see two possible alternatives here: either check for overflow and handle it gracefully or tighten the constraints on `Fraction` 's element type.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Fraction.h:100
+}
+} // namespace mlir
+#endif // MLIR_ANALYSIS_PRESBURGER_FRACTION_H
----------------
nit: Add blank lines before and after `} // namespace mlir`.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:9
+//
+// This a simplex 2D matrix class that supports reading, writing, resizing,
+// and swapping rows, and swapping columns.
----------------
`This a simplex 2D matrix class` -> `This is a simple 2D matric class`?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:10
+// This a simplex 2D matrix class that supports reading, writing, resizing,
+// and swapping rows, and swapping columns.
+//
----------------
nit: Remove redundant `and`.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:20
+#include <cassert>
+#include <vector>
+
----------------
If we are going to use vectors, `SmallVector` may be a better choice. Especially if you have an idea of the typical matrix size.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:27
+/// The data is stored in the form of a vector of vectors.
+template <typename INT>
+class Matrix {
----------------
I guess the `INT` means that the `Matrix` type should only be instantiated with integers. Maybe you could `static_assert` if it isn't the case or use SFINAE to constrain the possible template types.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:36
+
+  static Matrix getIdentityMatrix(unsigned dimension);
+
----------------
What about `identity`?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:66
+  unsigned nColumns;
+  std::vector<std::vector<INT>> data;
+};
----------------
Why not store the matrix data in a one-dimensional array?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:148
+void Matrix<INT>::print(llvm::raw_ostream &os) const {
+  os << "Dumping matrix, rows = " << getNumRows()
+     << ", columns: " << getNumColumns() << '\n';
----------------
This seems to be a very specific way to print a matrix. Do you need the `print` method or would `dump` be enough?
Also, I would probably not include the `Dumping matrix` part.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:151
+  os << "r/c  ";
+  for (unsigned column = 0; column < getNumColumns(); ++column)
+    os << "| " << column << " ";
----------------
This should probably be written as
```
for (unsigned column = 0, end = getNumColumns(); column < end; ++column)
```
(https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop)

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:155
+  os << std::string(5 + getNumColumns() * 5, '-') << '\n';
+  for (unsigned row = 0; row < getNumRows(); ++row) {
+    os << row << " | ";
----------------
Same comment as above.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:157
+    os << row << " | ";
+    for (unsigned column = 0; column < getNumColumns(); ++column)
+      os << data[row][column] << " ";
----------------
Same for this loop.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:168
+
+} // namespace mlir
+#endif // MLIR_ANALYSIS_PRESBURGER_MATRIX_H
----------------
nit: Add a space after this line.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:9
+//
+// This class can perform analysis on FlatAffineConstraints. In particular,
+// it can be used to perform emptiness checks.
----------------
nit: Since this is a top-level comment, maybe rephrase it to be less specific to the class. `This class` feels like it is too tied to a specific class in the file.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:44
+public:
+  enum class Direction { UP, DOWN };
+  enum class UndoOp {
----------------
Don't use SCREAMING_CASE for enumerators.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:47
+    DEALLOCATE,
+    UNMARK_EMPTY,
+  };
----------------
Same comment as above regarding the case.
Remove the trailing comma.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:56
+
+  /// \returns True if the tableau is empty (has conflicting constraints),
+  /// False otherwise.
----------------
Remove all the Doxygen tags in the file. There are quite a few places where you use `\returns` or `\p`.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:66
+  /// \returns the number of variables in the tableau.
+  unsigned numberVariables() const;
+
----------------
`numberVariables` -> `numVariables`?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:69
+  /// \returns the number of constraints in the tableau.
+  unsigned numberConstraints() const;
+
----------------
`numberConstraints` -> `numConstraints`?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:126
+    bool restricted;
+    unsigned pos;
+  };
----------------
nit: Reorder members by decreasing bit-width to limit padding.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:136
+  ///
+  /// \returns a [row, col] pair denoting a pivot, or an empty llvm::Optional if
+  /// no valid pivot exists.
----------------
The pair notation seems inconsistent with previous ones.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:138
+  /// no valid pivot exists.
+  llvm::Optional<std::pair<unsigned, unsigned>>
+  findPivot(int row, Direction direction) const;
----------------
Could `std::pair` be replaced by a struct with more descriptive member names?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:223
+  /// Holds a log of operations, used for rolling back to a previoous state.
+  std::vector<std::pair<UndoOp, llvm::Optional<int>>> undoLog;
+
----------------
This `vector` as well as the four that follow could be `SmallVector`s.

================
Comment at: mlir/lib/Analysis/AffineStructures.cpp:1051
+  Simplex simplex(*this);
+  return simplex.findIntegerSample();
+}
----------------
`return Simplex(*this).findIntegerSample();`?

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:25
+  for (unsigned i = 0; i < nVar; ++i) {
+    var.push_back(
+        Unknown(/*ownsRow=*/false, /*restricted=*/false, /*pos=*/nCol));
----------------
Replace `push_back` by `emplace_back`.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:34
+    : Simplex(constraints.getNumIds()) {
+  for (unsigned i = 0; i < constraints.getNumInequalities(); ++i)
+    addInequality(constraints.getInequality(i));
----------------
Same comment as above regarding the evaluation of the terminating condition, for this loop and the following one.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:42
+  assert(index != nullIndex && "nullIndex passed to unknownFromIndex");
+  return index >= 0 ? var[index] : con[~index];
+}
----------------
Why do you need Python-style access?

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:88
+
+  nRow++;
+  if (nRow >= tableau.getNumRows())
----------------
nit: `++nRow;`

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:91
+    tableau.resize(nRow, tableau.getNumColumns());
+  rowVar.push_back(~con.size());
+  con.emplace_back(true, false, nRow - 1);
----------------
This probably needs more documentation.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:128
+
+// Normalize the row by removing common factors that are common between the
+// denominator and all the numerator coefficients.
----------------
nit: Duplicate `common`.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:151
+// Find a pivot to change the sample value of `row` in the specified direction.
+// The returned pivot row will involve `row` if and only if the unknown is
+// unbounded in the specified direction.
----------------
nit: The usage of backticks here is inconsistent with previous code comments.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:183
+  auto opt = findPivotRow(row, newDirection, *col);
+  return std::pair<unsigned, unsigned>{opt.getValueOr(row), *col};
+}
----------------
If we choose to use `std::pair` then this should probably be a call to `std::make_pair`.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:392
+
+void Simplex::undoOp(UndoOp op, llvm::Optional<int> index) {
+  if (op == UndoOp::DEALLOCATE) {
----------------
This function seems to be a good candidate to be split into two different functions. The branch on the value of `op` seems very unnatural.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:481
+template <typename T>
+std::vector<T> concat(const std::vector<T> &v, const std::vector<T> &w) {
+  auto result = v;
----------------
This seems inefficient.
The function should have static linkage since it is only used as a helper in this file.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:878
+void Simplex::print(llvm::raw_ostream &os) const {
+  os << "Dumping Simplex, rows = " << nRow << ", columns = " << nCol << "\n";
+  if (empty)
----------------
Same comment as for the `Matrix::print` method.

================
Comment at: mlir/unittests/Analysis/Presburger/MatrixTest.cpp:89
+}
+} // namespace mlir
----------------
nit: Add a blank line above this one.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860