[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

Mon Jun 1 05:51:13 PDT 2020

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

Regarding the `print` and `dump` methods, I have no strong opinions. We may as well keep them as-is for now.
`Simplex.cpp` is apparently still using `//` for top-level comments.
Regarding the change to `SmallVector` instead of `std::vector`, it would probably make sense to use `ArrayRef<T>` for function parameters since it makes the API more flexible. If you need to mutate the parameter then I would suggest using `SmallVectorImpl<T>` instead of specifying the size of the internal buffer in the parameter type.

================
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;
+
----------------
arjunp wrote:
> 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?
`SmallVector` seems more appropriate since we can avoid allocations for small sizes. Do you expect to have a lot of 64-dimensional points? If not it may be a good idea to downsize the internal buffer to avoid wasting memory.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:20
+#include <cassert>
+#include <vector>
+
----------------
Do we need this include now that you went for `SmallVector`s?

================
Comment at: mlir/include/mlir/Analysis/Presburger/Matrix.h:66
+  unsigned nColumns;
+  std::vector<std::vector<INT>> data;
+};
----------------
arjunp wrote:
> 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.
If resizing is a common operation then I agree that it can make sense to keep nested `SmallVector`s. If there is a noticeable performance impact in can always be fixed in a follow-up.

================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:232
+
+  /// Holds a log of operations, used for rolling back to a previoous state.
+  llvm::SmallVector<std::pair<UndoOp, llvm::Optional<int>>, 64> undoLog;
----------------
`previoous` -> `previous`

================
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;
----------------
arjunp wrote:
> 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.
`PivotPair`, `PivotPoint`, or even simply `Pivot` all seem reasonable to me.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:42
+  assert(index != nullIndex && "nullIndex passed to unknownFromIndex");
+  return index >= 0 ? var[index] : con[~index];
+}
----------------
arjunp wrote:
> Kayjukh wrote:
> > Why do you need Python-style access?
> Sorry, I don't understand. Can you elaborate?
Forget about the "Python-style" part, I didn't notice that you accessed two different arrays depending on the index value. After re-reading the documentation comments I figured out why you can pass negative indices. Thanks for adding those explanations!

================
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);
----------------
arjunp wrote:
> 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?
It makes sense now. I probably overlooked the negative indices explanation the first time I went through your code.

================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:392
+
+void Simplex::undoOp(UndoOp op, llvm::Optional<int> index) {
+  if (op == UndoOp::DEALLOCATE) {
----------------
arjunp wrote:
> 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.
What I was suggesting was that the function could probably be split into two functions, namely `deallocate` and `unmarkEmpty`. Or is there a reason for doing a dispatch according to the `enum` value?

================
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;
----------------
arjunp wrote:
> 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.
In order to avoid potentially useless intermediate allocations, you could call `reserve` and then insert the elements. Something along the lines of
```
SmallVector<T, 64> result;
result.reserve(v.size() + w.size());
result.insert(adl_begin(result), adl_begin(v), adl_end(v));
result.insert(adl_end(result), adl_begin(w), adl_end(w));
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860