[PATCH] D80860: Exact integer emptiness checks for FlatAffineConstraints

[Arjun P via Phabricator via llvm-commits]

arjunp marked 3 inline comments as done.
arjunp added inline comments.


================
Comment at: mlir/include/mlir/Analysis/Presburger/Simplex.h:122
+/// Finding an integer sample is done with the Generalized Basis Reduction
+/// algorithm. See the documentation for findIntegerSample and reduceBasis.
+class Simplex {
----------------
mehdi_amini wrote:
> Nice doc for the overall class! Thanks :)
:)


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:866
+    addEquality(basisCoeffVector);
+    if (auto opt = findIntegerSampleRecursively(basis, level + 1))
+      return *opt;
----------------
mehdi_amini wrote:
> arjunp wrote:
> > 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.
> Please document clearly the bound on the recursion. What impacts the dimensionality of the set here? When you "is not expected to be greater than say 32", how can it grow larger?
It could theoretically grow arbitrarily large, although it may not happen in practice. I've made `findIntegerSample` iterative.


================
Comment at: mlir/lib/Analysis/Presburger/Simplex.cpp:944
+Optional<SmallVector<int64_t, 8>>
+Simplex::findIntegerSampleRecursively(Matrix &basis, unsigned level) {
+  if (level == basis.getNumRows())
----------------
bondhugula wrote:
> Please do not return `SmallVector`s this way - return value optimization here will not happen I believe leading to too many copies. Instead pass SmallVectorImpl<int64_t> by reference as an output argument. @mehdi_amini or @rriddle  can confirm on the style part and RVO.
(I think this is no longer applicable since `findIntegerSample` is iterative now)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80860