[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

[Arjun P via Phabricator via llvm-commits]

arjunp added a comment.

Thanks for your comments!

- Removed Doxygen tags.
- Used `///` for comments outside functions in header files.
- Used `SmallVector` instead `std::vector` everywhere.
- Removed the template parameters from `Fraction` and `Matrix`.



================
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;
+
----------------
Kayjukh wrote:
> `std::vector` is certainly not the right data structure to represent a point.
It's a point in an n-dimensional space. I changed it to a `SmallVector`. What would you recommend?


================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:66
+  unsigned nColumns;
+  std::vector<std::vector<INT>> data;
+};
----------------
Kayjukh wrote:
> Why not store the matrix data in a one-dimensional array?
This makes the code simpler. In that case, resizing would be more expensive, and resizing is not uncommon, so it's unclear to me if performance considerations weigh in favour of having it be one-dimensional.


================
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';
----------------
Kayjukh wrote:
> 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.
It seemed like `print` was generally declared whenever `dump` was. Should it be removed?


================
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;
----------------
Kayjukh wrote:
> Could `std::pair` be replaced by a struct with more descriptive member names?
Named it `pivotPair`. I wonder if there's a better name.


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


================
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);
----------------
Kayjukh wrote:
> This probably needs more documentation.
I added some more documentation to the Simplex class in the header explaining the tableau layout. Would that plus the documentation at the top of the definition be enough?


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:392
+
+void Simplex::undoOp(UndoOp op, llvm::Optional<int> index) {
+  if (op == UndoOp::DEALLOCATE) {
----------------
Kayjukh wrote:
> 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.
I could factor out the `Deallocate` branch's body into another function, but I don't understand how the branch can be completely eliminated.


================
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;
----------------
Kayjukh wrote:
> This seems inefficient.
> The function should have static linkage since it is only used as a helper in this file.
Made it static linkage. How can it be made more efficient? All the values need to be copied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860