[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.

Thanks very much for all the changes. A few more requests - a couple on test cases and other minor ones.



================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:454-458
+  while (Optional<Pivot> maybePivot = findPivot(row, direction)) {
+    if (maybePivot->row == row)
+      return {};
+    pivot(*maybePivot);
+  }
----------------
A short comment here please.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:575
+
+  SmallVector<int64_t, 8> sample;
+  for (const Unknown &u : var) {
----------------
Comment here please.


================
Comment at: mlir/unittests/Analysis/AffineStructuresTest.cpp:178
+
+TEST(FlatAffineConstraintsTest, IsIntegerEmptyTest) {
+  // 1 <= 5x and 5x <= 4 (no solution).
----------------
I think you are missing a test case involving equalities that is integer empty but not rational empty - a flat one along the lines of the test case added with purely inequalities. It could just be a 1-d line segment in a 2-d space that passes in between and thus misses all integer points - it could be potentially slanted as well.


================
Comment at: mlir/unittests/Analysis/Presburger/SimplexTest.cpp:16
+
+TEST(SimplexTest, emptyRollback) {
+  Simplex tab(2);
----------------
It may be good to add a line on what the test is testing.


================
Comment at: mlir/unittests/Analysis/Presburger/SimplexTest.cpp:22
+
+  auto snap = tab.getSnapshot();
+  // (u - v) <= -1
----------------
Avoid `auto` here please.


================
Comment at: mlir/unittests/Analysis/Presburger/SimplexTest.cpp:86-87
+
+TEST(SimplexTest, isUnbounded) {
+  EXPECT_FALSE(simplexFromInequalities(
+                   2, {{1, 1, 0}, {-1, -1, 0}, {1, -1, 5}, {-1, 1, -5}})
----------------
Missing a test case here that involves equalities?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860