This is an archive of the discontinued LLVM Phabricator instance.

Exact integer emptiness checks for FlatAffineConstraints
ClosedPublic

Authored by arjunp on May 29 2020, 11:19 PM.

Details

Summary

This patch adds the capability to perform exact integer emptiness checks for FlatAffineConstraints using the General Basis Reduction algorithm (GBR). Previously, only a heuristic was available for emptiness checks, which was not guaranteed to always give a conclusive result.

This patch adds a Simplex class, which can be constructed using a FlatAffineConstraints, and can find an integer sample point (if one exists) using the GBR algorithm. Additionally, it adds two classes Matrix and Fraction, which are used by Simplex.

The integer emptiness check functionality can be accessed through the new FlatAffineConstraints::isIntegerEmpty() function, which runs the existing heuristic first and, if that proves to be inconclusive, runs the GBR algorithm to produce a conclusive result.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

It looks like there are clang-format errors (https://results.llvm-merge-guard.org/amd64_debian_testing_clang-1651/summary.html). Can you format your changes using git-clang-format HEAD^ and update the diff? This would allow us to have a clean build.

arjunp updated this revision to Diff 267824.Jun 2 2020, 2:28 AM

Fix some whitespace and clang-format.

arjunp marked an inline comment as done and 3 inline comments as not done.Jun 2 2020, 2:40 AM
ftynse requested changes to this revision.Jun 3 2020, 3:23 PM

First of all, thanks for your contribution! This is an important and hard piece of polyhedral machinery.

The sheer size of the patch is huge and this will take some time to review. I only looked at the +/-style and simple issues and did not go into details of the algorithm itself. It would be helpful to know if you ran some code coverage tools on it.

Here is a list of major observations, which I did not always repeat:

  • I am concerned by 32 as default number of stack elements in SmallVector, do you have evidence for this specific number?
  • Please drop most of llvm:: prefixes, common classes are re-exported in the mlir:: namespace.
  • In general, all top-level entities as well as class methods must have documentation.
  • Consider making the API surface, both public and private, of classes smaller and using auxiliary functions with static linkage instead; same goes for multiple in-line lambdas that capture the world by-reference. These functions only need to be methods if they access class state, everything else can take that state as (const when possible) arguments and be cleaner about mutating it.
  • Some additional documentation on the practical implementation details is necessary, in particular, the ownership model on the tableau and the bitwise negation in indexing.

Please bear with us.

mlir/include/mlir/Analysis/AffineStructures.h
150

Multiple common LLVM ADT structures are re-exported in mlir namespace, see the full list here https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Support/LLVM.h. We generally avoid prefixing these with llvm:: or mlir:: for brevity.

mlir/include/mlir/Analysis/Presburger/Fraction.h
18

Prefer <cstdint> in C++ code for namespacing reasons

22

This needs a doc

34

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?

mlir/include/mlir/Analysis/Presburger/Matrix.h
36

All public methods should have at least some documentation.

66

This rationale should be documented in the code. The interplay between resizes and cache behavior in case of consecutive reads (which I suppose are much more common) is non-trivial, especially if we allow reserve-ing space. I'm also slightly concerned about using 8k bytes on stack by default, we rarely use more than 8 stack elements in small vectors in MLIR.

148

Please keep the print. It allows one to print objects into streams different than llvm::errs().

mlir/include/mlir/Analysis/Presburger/Simplex.h
37–41

This comment lost me about what are unknowns, variables and constraints, and what is the relation between them, even though I know the theory behind this.

47

Could you please elaborate a little about this bit-completent indexing, e.g. that -1 gets transformed into 0 and -2 into 1 and so on, sparing you an addition. This is a clever trick, but clever tricks are very expensive to maintain

56

Nit: suffix Op is pervasively used for IR Operations, please consider a different name

96

Nit: null value -> None

113

Nit: empty llvm::Optional -> None, here and below

126

Extra points: use a bit field or a uint8_t for flags

131

Nit: -1u (or UINT_MAX if you prefer)

133

Please, provide documentation about this class. Especially the ownership model and how it is maintained.

187

Prefer mlir::LogicalResult for success/failure conditions to avoid deadling with LLVM's changing mapping of boolean values to success status

203

Nit: true _if_ value is positive

218

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 ?

237

Why is this int when pretty much everything else uses either unsigned or int64_t?

mlir/lib/Analysis/Presburger/Matrix.cpp
18 ↗(On Diff #267824)

unsigned is used everywhere else

76 ↗(On Diff #267824)

nit: use single quotes for one character

mlir/lib/Analysis/Presburger/Simplex.cpp
2

Drop "C++" in .cpp files, it's only necessary for headers (.h) so that tooling can tell between C and C++ that share the extension

15

Is this include necessary?

417

Please use braces symmetrically, i.e. use them if at least one if/else block needs them

534

Why does this copy the first two columns from A?

563

Please use braces symmetrically; here and below

591

Use SmallVectorImpl with references so you don't need to hardcode the size

607

Nit: consider a ternary expression here

614

This class needs a better description of what are the invariants on the snapshot stack. This method rolls back one snapshot, which happens to be be before an equality was added, but without proper documentation, there is a risk this might be broken in future.

628

Don't evaluate the exit condition on every iteration, use

for (unsigned i = 0, e = dir.size(); i < e; ++i)

697

Please consider a lengthier name for this and all derived objects, e.g. fAtI

758

Nit: asserts are for things that should never happen, no need to mention it in the description

834

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

867

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.

883

Nit: print may be used outside dump, so I'd abstain from refering to "Dumping" here

This revision now requires changes to proceed.Jun 3 2020, 3:23 PM
arjunp updated this revision to Diff 268421.Jun 4 2020, 4:32 AM

Add two tests of "long" sets. Fix some bugs that this revealed.

bondhugula requested changes to this revision.EditedJun 5 2020, 12:25 AM

I am happy to see this submitted for review. An exact integer emptiness check had been missing and having one would be a really useful contribution. Although expensive in theory, for many of the use cases here (which are typically low / fixed dimensionality ones), things may be practical and fast enough. It just has be carefully used so as to not be detrimental to compile time for "standard" enough use cases - since there may be alternative faster approaches. It's nevertheless useful to have this technique at our disposal and have this maintained and optimized for use cases of interest, including for experimental/research purposes.

I strongly agree with all of @ftynse's comments; they already cover pretty much all of what I may have wanted to say. Just one additional comment below on the storage for Matrix.

mlir/include/mlir/Analysis/Presburger/Matrix.h
66

Please avoid using vector of vector or SmallVector of SmallVector here. (A SmallVector isn't really optimized to hold other vectors or SmallVectors as its elements.) All of these would be bad enough for performance to warrant using a 1-d storage to start with. Using a single (contiguous) buffer for all the coefficients (a single allocation holding it) will give you cache locality (spatial), fewer/no conflict misses, better prefetching, and fewer TLB misses (just one in nearly all cases). You'll also get completely inline / stack allocations for small matrices. Your API won't be really impacted. FlatAffineConstraints uses such a single buffer of coefficients via a SmallVector (you can actually copy over its relevant methods - instead of equalities and inequalities, there's just coefficients here.) Resizing isn't really a downside with this. Adding rows is straightforward since they are at the tail, and adding columns will require movement within the contiguous buffer, which is fine. FAC reserves columns to reduce movement most of the time for column addition. With that, I don't see any additional resizing or movement penalty with a 1-d buffer over vector of vectors - you have too many allocations with the latter.

This revision now requires changes to proceed.Jun 5 2020, 12:25 AM
bondhugula added inline comments.Jun 5 2020, 12:33 AM
mlir/include/mlir/Analysis/Presburger/Matrix.h
66

Esp. for mid-size matrices like 10x10 or 20x20, all of the allocs and deallocs from a smallvector of smallvector will be a heap killer!

mehdi_amini added inline comments.Jun 5 2020, 12:59 AM
mlir/lib/Analysis/Presburger/Simplex.cpp
628

or better with range-style: for (int i : llvm::seq<int>(dir.size())) :)

arjunp marked 52 inline comments as done.Jun 11 2020, 2:31 PM

Thanks for your review. Apologies for the delayed response.

  • Here is a code coverage report.
  • The lengths of these SmallVectors 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.
mlir/include/mlir/Analysis/Presburger/Fraction.h
34

Added documentation.

56

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?

mlir/include/mlir/Analysis/Presburger/Simplex.h
218

I move flippedDirection and signMatchesDirection out into free functions. reduceBasis does need access to this, but only to construct the GBRSimplex.

237

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.

mlir/lib/Analysis/Presburger/Simplex.cpp
498

Pulling this out would require making Simplex::Unknown public or templating on the store type.

534

Nice catch. Fixed.

607

(Not applicable anymore.)

697

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

834

It needs access to this for computeOptimum. Factored out into a member function computeIntegerBounds.

867

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.

arjunp updated this revision to Diff 270240.Jun 11 2020, 2:33 PM
arjunp marked 5 inline comments as done.
This comment was removed by arjunp.
mehdi_amini added inline comments.Jun 11 2020, 8:51 PM
mlir/lib/Analysis/Presburger/Simplex.cpp
867

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?

mehdi_amini added inline comments.Jun 11 2020, 8:57 PM
mlir/include/mlir/Analysis/Presburger/Simplex.h
118

s/particlar/particular/

123

Nice doc for the overall class! Thanks :)

mlir/lib/Analysis/Presburger/Simplex.cpp
474

Please avoid auto (here and everywhere applicable) when it doesn't improve readability (unsigned here right?)

bondhugula requested changes to this revision.Jun 12 2020, 3:37 AM
bondhugula added inline comments.
mlir/lib/Analysis/Presburger/Simplex.cpp
908

Please avoid auto here and similarly below.

945

Please do not return SmallVectors 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.

mlir/unittests/Analysis/AffineStructuresTest.cpp
35

Avoid auto here for readability.

39

Avoid auto.

115

Please terminate comments with a period when it ends.

This revision now requires changes to proceed.Jun 12 2020, 3:37 AM
bondhugula added inline comments.Jun 12 2020, 3:42 AM
mlir/lib/Analysis/Presburger/Simplex.cpp
74

Nit: You could leave the first three lines as the doc comment and move the ones below inline into the method as regular comments.

arjunp updated this revision to Diff 270901.Jun 15 2020, 4:15 PM
arjunp marked 5 inline comments as done.

Address comments. Made findIntegerSample iterative.

arjunp marked 3 inline comments as done.Jun 15 2020, 4:19 PM
arjunp added inline comments.
mlir/include/mlir/Analysis/Presburger/Simplex.h
123

:)

mlir/lib/Analysis/Presburger/Simplex.cpp
867

It could theoretically grow arbitrarily large, although it may not happen in practice. I've made findIntegerSample iterative.

945

(I think this is no longer applicable since findIntegerSample is iterative now)

bondhugula requested changes to this revision.Jun 15 2020, 8:05 PM

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

mlir/unittests/Analysis/AffineStructuresTest.cpp
179

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.

mlir/unittests/Analysis/Presburger/SimplexTest.cpp
17

It may be good to add a line on what the test is testing.

87–88

Missing a test case here that involves equalities?

bondhugula added inline comments.Jun 15 2020, 8:05 PM
mlir/lib/Analysis/Presburger/Simplex.cpp
455–459

A short comment here please.

576

Comment here please.

mlir/unittests/Analysis/Presburger/SimplexTest.cpp
23

Avoid auto here please.

This revision now requires changes to proceed.Jun 15 2020, 8:05 PM
bondhugula added a comment.EditedJun 15 2020, 8:13 PM

Not for this patch, but if you want to just quickly try and test it on more cases, you could just replace mlir::boundCheckLoadOrStoreOp's isEmpty() with isIntegerEmpty() and see if -test-memref-bound-check test cases pass. The -test-memref-bound-check pass tests for out of bound memref accesses and actually needs an integer exact method (whenever it determines non-empty) to be accurate. This way you would also have a speed comparison, and it would also be a practical application of this technique for an analysis pass.

tests/Transforms/memref-bound-check.mlir

arjunp updated this revision to Diff 271040.Jun 16 2020, 4:19 AM
arjunp marked 6 inline comments as done.

Addressed comments.

I've added some more tests and documentation as requested.

bondhugula added inline comments.Jun 17 2020, 12:01 AM
mlir/unittests/Analysis/AffineStructuresTest.cpp
187

I'm sorry this would be caught by the GCD test itself. Instead, could you please replace the equality with something to the effect:

3x - 3y >= -2
3x - 3y  <= -1

This should be empty as well but would exercise your method given how isIntegerEmpty() is structured now.

bondhugula added inline comments.Jun 17 2020, 12:50 AM
mlir/include/mlir/Analysis/Presburger/Fraction.h
66

Are you assuming f.den is always positive? If both num and den are both negative, you'd want to add one.

arjunp updated this revision to Diff 271290.Jun 17 2020, 12:53 AM
arjunp marked 2 inline comments as done.

Added the requested test.

arjunp updated this revision to Diff 271292.Jun 17 2020, 12:55 AM

Fix a comment in Fraction.

arjunp marked 2 inline comments as done.Jun 17 2020, 12:57 AM
arjunp added inline comments.
mlir/include/mlir/Analysis/Presburger/Fraction.h
66

Yes, den is always positive. There was an incomplete comment for the member declaration that I've fixed. (This is also documented in the top-level class documentation)

mlir/unittests/Analysis/AffineStructuresTest.cpp
187

Since checkSample directly calls findIntegerSample() rather than isIntegerEmpty(), this shouldn't be an issue. Nonetheless, I've also added the test you requested.

Harbormaster completed remote builds in B60595: Diff 271292.
ftynse accepted this revision.Jun 22 2020, 6:00 AM

Thanks for providing more detailed documentation!

I did not extensively analyze the algorithms. At a glance, the implementation looks plausible. Again, given the complexity, I would encourage you to get 100% test coverage on the core functions. Another way to stress-test this would be to generate a bunch of arbitrary sets and run this + gcd emptiness checks and analyze the differences manually to make sure they only happen when gcd is not supposed to be correct.

I don't have any further comments.

@andydavis1 had a bug which might have been related to integer exactness, but I don't know if it is still active.

mlir/include/mlir/Analysis/Presburger/Simplex.h
38

Nit: SImplex -> simplex

46

Nit: 2x - 3y >= 0 (currently uses z)

bondhugula requested changes to this revision.Jun 22 2020, 8:40 AM
bondhugula added inline comments.
mlir/include/mlir/Analysis/Presburger/Fraction.h
66

If den is positive, this can be simplified to avoid an extra check.

f.num % f.den > 0 ? f.num / f.den + 1 : f.num / f.den;

(one comparison instead of two comparisons)

Also, if there is a doc comment here, please mention that den is positive. Actually, you don't even need this logic; mlir::ceilDiv does exactly this.

This revision now requires changes to proceed.Jun 22 2020, 8:40 AM

Thanks for providing more detailed documentation!

I did not extensively analyze the algorithms. At a glance, the implementation looks plausible. Again, given the complexity, I would encourage you to get 100% test coverage

+1 I don't think it's practical to have a review checking each of the steps of the algorithm. I would just rely on (1) good test case coverage, and (2) sufficient documentation/comments for now. Other implementation aspects can be brought up and revised later when there are other users with a specialized interest in digging deeper.

Thanks for your contribution!

It would be great to have tests on integer sets which resulting in an over approximation using the existing FM algorithm, but which returned integer exact results using GBR from this change.

Also wondering what the plan is to check for integer overflow of coefficient values for some of these algorithms?

arjunp updated this revision to Diff 273566.EditedJun 25 2020, 5:25 PM
arjunp marked 4 inline comments as done.

Improved test coverage. There is still one uncovered line; it seems quite difficult to find a case that covers this. I ran hundreds of tests with randomly generated sets intersected with a 1000 x 1000 x 1000 cube. Whenever GBR and the FM/GCD-based algorithm disagreed, I checked against a brute-force that checks every point in this cube. All these tests passed, as well as tests from some other types of generated cases.

Added some tests that GBR detects as empty but the existing FM/GCD-based algorithm does not.

Regarding overflow issues, it does not seem to be something specific to this patch. It looks to me like there already exists other functionality in FlatAffineConstraints, like FM, that performs operations without complete overflow checks. I feel that all these need to be addressed together by, say, introducing arbitrary-precision arithmetic, which is beyond the scope of this patch.

Thanks again for all your help with improving this patch!

bondhugula accepted this revision.Jun 27 2020, 3:12 AM
bondhugula added inline comments.Jun 27 2020, 3:30 AM
mlir/include/mlir/Analysis/Presburger/Fraction.h
39

This has to be a /// comment.

Added some tests that GBR detects as empty but the existing FM/GCD-based algorithm does not.

It'll be useful if you marked these tests with a comment to that effect. I might have missed it if you already did.

bondhugula requested changes to this revision.Jun 28 2020, 1:22 AM

A bunch of mostly minor comments.

mlir/lib/Analysis/Presburger/Simplex.cpp
707

Can you rephrase this? <dir, x-y> is unclear.

711

Please use coeffs.reserve since you know big it's going to be.

712–714

Why emplace_back instead of push_back for a POD?

719–723

Triple comments please.

866–867

Reflow - use the width?

875

Please make this a complete sentence.

961

You can use llvm::to_vector.

1011

llvm::to_vector

mlir/unittests/Analysis/AffineStructuresTest.cpp
30

Nit: hasValue -> 'hasValue'

56

SmallVector -> ArrayRef

57

Likewise.

68

Typo: least

72–73

Please pass ArrayRef instead of SmallVector for inputs. (If these were output args or inouts, SmallVectorImpl is to be used.)

mlir/unittests/Analysis/Presburger/SimplexTest.cpp
54

SmallVector 4?

60

Likewise.

This revision now requires changes to proceed.Jun 28 2020, 1:22 AM
arjunp updated this revision to Diff 274666.EditedJun 30 2020, 7:08 PM
arjunp marked 16 inline comments as done.

Addressed comments.

arjunp marked an inline comment as done.Jun 30 2020, 7:11 PM

Added some tests that GBR detects as empty but the existing FM/GCD-based algorithm does not.

It'll be useful if you marked these tests with a comment to that effect. I might have missed it if you already did.

This is marked at AffineStructuresTest.cpp:247.

mlir/lib/Analysis/Presburger/Simplex.cpp
707

I've replaced it with the more explicit dotProduct everywhere now. (this was originally explained in the top-level class comment)

mlir/unittests/Analysis/AffineStructuresTest.cpp
65

Is there a better way? This is needed since we moved to ArrayRefs for arguments but in checkPermutationsSample below we need to pass SmallVectors of SmallVectors to makeFACFromConstraints.

bondhugula added inline comments.Jul 1 2020, 12:04 AM
mlir/unittests/Analysis/AffineStructuresTest.cpp
65

We don't need to switch to ArrayRef of ArrayRef if you have SmallVector of SmallVector, but only to ArrayRef of SmallVector.

bondhugula accepted this revision.Jul 1 2020, 12:06 AM
bondhugula added inline comments.
mlir/unittests/Analysis/AffineStructuresTest.cpp
65

I actually meant ArrayRef<SmallVector,...> as the function argument instead of SmallVector<SmallVector<.... Make sure you don't run into underlying storage lifetime issues. ArrayRef<SmallVector..> should be fine.

arjunp updated this revision to Diff 274730.Jul 1 2020, 2:43 AM

Switched from ArrayRef of ArrayRef to ArrayRef of SmallVectors.

ftynse added a comment.Jul 1 2020, 3:25 AM

@Kayjukh your review is blocking this revision, have your concerns been addressed?

Kayjukh accepted this revision.Jul 1 2020, 3:53 AM

Yes, sorry about blocking the revision. Note that there is still a clang-format issue in one of the files.

mlir/unittests/Analysis/AffineStructuresTest.cpp
89

Please fix formatting

This revision is now accepted and ready to land.Jul 1 2020, 3:53 AM
arjunp updated this revision to Diff 274760.Jul 1 2020, 4:44 AM

clang-format.

arjunp marked an inline comment as done.Jul 2 2020, 6:23 AM

I don't have commit access yet, so I think someone else will need to land this.

I don't have commit access yet, so I think someone else will need to land this.

I've committed this.

This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jan 2 2022, 9:27 PM
mlir/lib/Analysis/Presburger/Simplex.cpp
132

What guarantees that gcd can't be 0 and produce an invalid division here?
Should we guard this loop?

(flagged by Coverity)

mehdi_amini added inline comments.Jan 2 2022, 10:14 PM
mlir/lib/Analysis/Presburger/Simplex.cpp
326

If retRow is true, then retConst is used uninitialized right? Can you look into this?
(flagged by Coverity)

326

(Same with retElem)

arjunp added inline comments.Jan 5 2022, 9:29 AM
mlir/lib/Analysis/Presburger/Simplex.cpp
132

One of the elements being GCD'd is a "denominator" value that is always non-zero. I will add a comment noting this.

326

If retRow has a value then lines 319-322 were executed in the iteration that that values was set in, so the values of retElem and retConst were also set in that iteration already.

Hi @arjunp, I am wondering if there are ways to prevent future coverity warnings on this code?

mlir/lib/Analysis/Presburger/Simplex.cpp
132

Given that these are coverity errors, it might be useful to document this with an assert to ensure that coverity does not warn about this anymore.

326

Again, as these are coverity errors the question is how we can avoid coverity to flag these issues. I am almost tempted to just suggest to set retElement and retConst to 0 and add a comment stating that this is done to avoid the coverity warning.