[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

[Arjun P via Phabricator via llvm-commits]

arjunp marked 52 inline comments as done.
arjunp added a comment.

Thanks for your review. Apologies for the delayed response.

- Here <https://superty.github.io/gbr-coverage/coverage/home/arjun/llvm-project/mlir/lib/Analysis/Presburger/Simplex.cpp.html> is a code coverage report.
- The lengths of these `SmallVector`s corresponds to the dimensionality. I've changed the length parameter to 8 for vectors and 64 for the 2D matrix, as it is in `FlatAffineConstraints`.
- The lambdas in `makeProduct` and `reduceBasis` are very specific to these functions and access a lot of the local variables present in those functions. Does it still make sense to pull these out into free functions?
- Added more documentation for the `Simplex` class.



================
Comment at: mlir/include/mlir/Analysis/Presburger/Fraction.h:33
+inline int compare(Fraction x, Fraction y) {
+  int64_t diff = x.num * y.den - y.num * x.den;
+  if (diff > 0)
----------------
ftynse wrote:
> This may still be subject to overflows. I'm fine with documenting this behavior somewhere and leaving as is, but we need to be aware of the problem. Have you considered performance implications of basic Fraction on APInt instead of int64?
Added documentation.


================
Comment at: mlir/include/mlir/Analysis/Presburger/Fraction.h:55
+
+inline Fraction operator-(Fraction x) { return Fraction(-x.num, x.den); }
+
----------------
I didn't document these overloads as it's unclear to me what the documentation could add. Is it necessary to write something for these?


================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:217
+  /// basis reduction.
+  void reduceBasis(Matrix &basis, unsigned level);
+
----------------
ftynse wrote:
> General design question: how much of these functions need to be private methods, i.e. have access to class members, as opposed to be just free functions, static to the compilation unit, that actual methods can call ?
I move `flippedDirection` and `signMatchesDirection` out into free functions. `reduceBasis` does need access to `this`, but only to construct the `GBRSimplex`. 


================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:236
+  /// These hold the indexes of the unknown at a given row or column position.
+  llvm::SmallVector<int, 32> rowVar, colVar;
+
----------------
ftynse wrote:
> Why is this `int` when pretty much everything else uses either unsigned or int64_t?
Added documentation. It's not `unsigned` because it's convenient to use sign comparisons to determine if an index corresponds to a constraint or a variable.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:497
+
+  auto concat = [](llvm::ArrayRef<Unknown> v, llvm::ArrayRef<Unknown> w) {
+    llvm::SmallVector<Unknown, 32> result;
----------------
Pulling this out would require making `Simplex::Unknown` public or templating on the store type.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:533
+  auto appendRowFromB = [&](unsigned row) {
+    result.tableau(result.nRow, 0) = a.tableau(row, 0);
+    result.tableau(result.nRow, 1) = a.tableau(row, 1);
----------------
ftynse wrote:
> Why does this copy the first two columns from A?
Nice catch. Fixed.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:606
+        // The dual variable is the negative of the row coefficient.
+        dual.push_back(-simplex.tableau(row, simplex.con[i].pos));
+      }
----------------
ftynse wrote:
> Nit: consider a ternary expression here
(Not applicable anymore.)


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:696
+  GBRSimplex gbrSimplex(*this);
+  llvm::SmallVector<Fraction, 32> f;
+  llvm::SmallVector<int64_t, 32> alpha;
----------------
ftynse wrote:
> Please consider a lengthier name for this and all derived objects, e.g. fAtI
Changed to `width`. It's not strictly speaking the width but the width under some constraints, but I guess this is a good enough approximation. (and it is already referred to as "width" in `computeWidthAndDuals`).


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:833
+
+  auto getBounds = [&]() -> std::pair<int64_t, int64_t> {
+    int64_t minRoundedUp;
----------------
ftynse wrote:
> This function would be cleaner if it took `opt` as argument and did not capture the world by-reference. It could then also go outside as a helper with static linkage
It needs access to `this` for `computeOptimum`. Factored out into a member function `computeIntegerBounds`.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:866
+    addEquality(basisCoeffVector);
+    if (auto opt = findIntegerSampleRecursively(basis, level + 1))
+      return *opt;
----------------
ftynse wrote:
> MLIR generally discourages recursive functions on anything that runs on pieces of IR, stack overflows in the compiler are bad. Any chance of making this iterative? Otherwise, please provide some textual argument on how fast the recursion stops.
The depth of the recursion is at most the dimensionality of the set, which is not expected to be greater than say 32. This could be made iterative, albeit with some loss in readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860