[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

[Uday Bondhugula via Phabricator via llvm-commits]

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

A bunch of mostly minor comments.



================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:706
+private:
+  // Returns coefficients for the expression <dir, x - y>.
+  SmallVector<int64_t, 8> getCoeffsForDirection(ArrayRef<int64_t> dir) {
----------------
Can you rephrase this? <dir, x-y> is unclear.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:710
+           "Direction vector has wrong dimensionality");
+    SmallVector<int64_t, 8> coeffs(dir.begin(), dir.end());
+    for (int64_t coeff : dir)
----------------
Please use coeffs.reserve since you know big it's going to be.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:711-713
+    for (int64_t coeff : dir)
+      coeffs.emplace_back(-coeff);
+    coeffs.emplace_back(0); // constant term
----------------
Why `emplace_back` instead of `push_back` for a POD?


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:718-722
+  // The first index of the equality constraints, the index immediately after
+  // the last constraint in the initial product simplex.
+  unsigned simplexConstraintOffset;
+  // A stack of snapshots, used for rolling back.
+  SmallVector<unsigned, 8> snapshotStack;
----------------
Triple comments please.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:865-866
+      assert(i + 1 >= level + width.size() &&
+             "We don't know dual_i but we know "
+             "width_{i+1}");
+      // We don't know dual for our level, so let's find it.
----------------
Reflow - use the width?


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:874
+
+    // width_i(b_{i+1} + u*b_i)
+    Fraction widthICandidate = updateBasisWithUAndGetFCandidate(i);
----------------
Please make this a complete sentence.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:960
+      // generalized basis reduction.
+      SmallVector<int64_t, 8> basisCoeffs(basis.getRow(level).begin(),
+                                          basis.getRow(level).end());
----------------
You can use `llvm::to_vector`.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:1010
+    // Try the next value in the range and "recurse" into the next level.
+    SmallVector<int64_t, 8> basisCoeffs(basis.getRow(level).begin(),
+                                        basis.getRow(level).end());
----------------
`llvm::to_vector`


================
Comment at: mlir/unittests/Analysis/AffineStructuresTest.cpp:29
+
+/// If hasValue is true, check that findIntegerSample returns a valid sample
+/// for the FlatAffineConstraints fac.
----------------
Nit: hasValue -> 'hasValue'


================
Comment at: mlir/unittests/Analysis/AffineStructuresTest.cpp:55
+makeFACFromConstraints(unsigned dims,
+                       SmallVector<SmallVector<int64_t, 4>, 4> ineqs,
+                       SmallVector<SmallVector<int64_t, 4>, 4> eqs) {
----------------
SmallVector -> ArrayRef


================
Comment at: mlir/unittests/Analysis/AffineStructuresTest.cpp:56
+                       SmallVector<SmallVector<int64_t, 4>, 4> ineqs,
+                       SmallVector<SmallVector<int64_t, 4>, 4> eqs) {
+  FlatAffineConstraints fac(ineqs.size(), eqs.size(), dims + 1, dims);
----------------
Likewise.


================
Comment at: mlir/unittests/Analysis/AffineStructuresTest.cpp:67
+/// constraint set. Since the GBR algorithm progresses dimension-wise, different
+/// orderings may cause the algorithm to proceed differently. At lesast some of
+///.these permutations should make it past the heuristics and test the
----------------
Typo: least


================
Comment at: mlir/unittests/Analysis/AffineStructuresTest.cpp:71-72
+void checkPermutationsSample(bool hasValue, unsigned nDim,
+                             SmallVector<SmallVector<int64_t, 4>, 4> ineqs,
+                             SmallVector<SmallVector<int64_t, 4>, 4> eqs) {
+  SmallVector<unsigned, 4> perm(nDim);
----------------
Please pass ArrayRef instead of SmallVector for inputs. (If these were output args or inouts, SmallVectorImpl is to be used.)


================
Comment at: mlir/unittests/Analysis/Presburger/SimplexTest.cpp:53
+  Simplex simplex(3);
+  std::vector<int64_t> coeffs[]{{1, 0, 0, 0},   // u >= 0.
+                                {-1, 0, 0, 0},  // u <= 0.
----------------
SmallVector 4?


================
Comment at: mlir/unittests/Analysis/Presburger/SimplexTest.cpp:59
+  // The constraints below violate v = w.
+  std::vector<int64_t> checkCoeffs[]{{0, 1, -1, -1},  // v - w >= 1.
+                                     {0, -1, 1, -1}}; // v - w <= -1.
----------------
Likewise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860