This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] IntegerPolyhedron: add support for symbolic integer lexmin
ClosedPublic

Authored by arjunp on Apr 2 2022, 1:12 PM.

Details

Summary

Add support for computing the symbolic integer lexmin of a polyhedron.
This finds, for every assignment to the symbols, the lexicographically
minimum value attained by the dimensions. For example, the symbolic lexmin
of the set

(x, y)[a, b, c] : (a <= x, b <= x, x <= c)

can be written as

x = a if b <= a, a <= c
x = b if a <  b, b <= c

This also finds the set of assignments to the symbols that make the lexmin unbounded.

Diff Detail

Event Timeline

arjunp created this revision.Apr 2 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2022, 1:12 PM
arjunp requested review of this revision.Apr 2 2022, 1:12 PM
arjunp updated this revision to Diff 420009.Apr 2 2022, 1:32 PM

Fix some issues I just noticed

Some initial comments which may change the implementation a lot (in case we agree).

mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
618

I would prefer not treating the relation as a set while producing this lexmin. Can we instead also treat the domain variables as parameters and have something like:

(d0, d1) -> (r1, r2)[a, b, c] : ...

gives

Domain: (d0, d1)[a, b, c] : ...
Values of range: r1 = .., r2 = ..

...

I think this is a more general case of what the current implementation produces.

620–623

I find the documentation incomplete. What is symbolDomain? Does it only contain symbols? Does it have to belong to the same space as the relation?

mlir/include/mlir/Analysis/Presburger/PWMAFunction.h
111
175
Groverkss added inline comments.Apr 2 2022, 3:41 PM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
618

Just to clarify, I wanted this to be moved to IntegerRelation as a general case. I'm not sure if these changes can be easily accommodated later.

arjunp added inline comments.Apr 2 2022, 4:52 PM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
618

Yeah, that can be done "easily", i.e., without changes to the Simplex. I mostly put it in the set and not relation for now because I didn't want to think of exactly what interface would be exposed.

The only thing is, I think we might have to change MultiAffineFunction and PWMAFunction to use IntegerRelation and that's a pretty big find-replace change. These are interface changes that do not affect the underlying algorithm. You can tell SymbolicLexSimplex which variables are to be treated as "symbols" and which as "non-symbols". Currently, only the symbols in the space are treated as symbols and the rest as non-symbols, but this can easily be changed later.

arjunp updated this revision to Diff 420021.Apr 2 2022, 5:01 PM
arjunp marked an inline comment as done.

Remove unneeded overload

arjunp updated this revision to Diff 420022.Apr 2 2022, 5:22 PM

Add some documentation and asserts to the SymbolicLexSimplex constructor

arjunp updated this revision to Diff 420023.Apr 2 2022, 5:25 PM

Fix some documentation

Groverkss added inline comments.Apr 3 2022, 2:57 AM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
618

Why would you need to change MAF and PWMA to IntegerRelation? Right now your domain of function will look something like:

()[a, b, c] : ...

After this change, it will be:

(d0, d1)[a, b, c] : ...
618

Can you also tell which IdKind the symbol was after finding it's lexmin? I think that would be needed to distinguish domain variables vs symbol variables.

arjunp added inline comments.Apr 3 2022, 9:32 AM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
618

Right, if we go with defining the domain like that it'll be fine I guess. At the moment the symbols of the set are the dimensions of the function's domain.

618

The order of symbol IDs will be the same on output, so first the domain IDs, then the symbol, then the locals created by the symbolic lexmin over these. So we can always find out. Also, we could just put symbols as symbols and domain as dims in the space of the symbolDomain provided to SymbolicLexMin, so the resulting function's domain will have the same space

Another pass of comments.

mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
618

Can we for this patch have the domain of PWMA treat symbols as symbols only instead of converting them to dimensions?

mlir/include/mlir/Analysis/Presburger/Simplex.h
219–221

Instead of passing these parameters, you could also pass a PresburgerSpace to this and extract this information from it.

581–582

I don't like this conversion to dim id. Why can't they just be symbols?

arjunp marked 2 inline comments as done.Apr 3 2022, 2:45 PM
arjunp added inline comments.
mlir/include/mlir/Analysis/Presburger/Simplex.h
219–221

The "symbols" here are not necessarily the symbols of the input set. For example, if we want the output to be a function of the domains and symbols of the input, we would pass both domain and symbols as symbols here. In general, these may not even form a a continuous range, in which case we can consider either passing a BitVector denoting which vars are symbols, or indeed passing a PresburgerSpace and a list of IdKinds to be considered symbols. These things are easily changed later.

581–582

It seems more natural to me to think of the result as being a function with its domain corresponding to the symbols of the input set and its range corresponding to the non-symbols of the input set, as opposed to the result being a family of functions parametrized by the symbols of the input sets and with no domain.

arjunp updated this revision to Diff 420091.Apr 3 2022, 2:46 PM

TODO -> TODO:

Another pass of comments.

mlir/include/mlir/Analysis/Presburger/Matrix.h
154–155

This is a bit confusing. Can you rephrase it to say that the length of elems should be equal to nColumns?

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

Are you missing a full stop (.) here?

mlir/lib/Analysis/Presburger/Simplex.cpp
346–348

You can maybe make the divisor unsigned and add an assert to check if it positive.

mlir/unittests/Analysis/Presburger/IntegerPolyhedronTest.cpp
1144

Remove this debug statement please.

Groverkss added inline comments.Apr 4 2022, 4:34 AM
mlir/include/mlir/Analysis/Presburger/Simplex.h
219–221

Ok it seems like we can do these things after this patch.

581–582

I guess it's fine for now and we can discuss this again later as it seems to be easy to change.

arjunp updated this revision to Diff 420156.Apr 4 2022, 6:13 AM
arjunp marked 13 inline comments as done.

Address comments

mlir/lib/Analysis/Presburger/Simplex.cpp
346–348

I added the assert in isRangeDivisbleBy since I think it's better for it to assert itself than make its users assert (users are typically going to have an int64_t)

arjunp updated this revision to Diff 420157.Apr 4 2022, 6:16 AM

Remove use of auto

arjunp updated this revision to Diff 420159.Apr 4 2022, 6:21 AM

fix expression in symbolic cut documentation

Another pass of comments.

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

Should this be isSymbol?

397
mlir/lib/Analysis/Presburger/Simplex.cpp
34

The assert should be at top of function body I think.

346–348

Sorry if I was ambiguous in what I mean. I meant the same thing that you did here :P

389–412

Could you add some inline explanation (above each logical block of code maybe) of what is happening here?

mlir/unittests/Analysis/Presburger/IntegerPolyhedronTest.cpp
1159–1162

Why std::cerr instead of llvm::errs?

arjunp updated this revision to Diff 420265.Apr 4 2022, 12:05 PM
arjunp marked 6 inline comments as done.

Address comments and add a bit more doc

This revision is now accepted and ready to land.Apr 4 2022, 12:21 PM
This revision was landed with ongoing or failed builds.Apr 4 2022, 4:25 PM
This revision was automatically updated to reflect the committed changes.

Please include revert reason in the commit message when you revert in the future.

arjunp added a comment.Apr 4 2022, 4:50 PM

Please include revert reason in the commit message when you revert in the future.

Okay, will do. Sorry about that!