This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] Support computing a representation of a set that only has locals that are divs
ClosedPublic

Authored by arjunp on Jun 9 2022, 6:43 PM.

Details

Summary

This paves the way for integer-exact projection, and for supporting
non-division locals in subtraction, complement, and equality checks.

Diff Detail

Event Timeline

arjunp created this revision.Jun 9 2022, 6:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 6:43 PM
arjunp requested review of this revision.Jun 9 2022, 6:43 PM
Groverkss added inline comments.Jun 10 2022, 4:54 AM
mlir/lib/Analysis/Presburger/IntegerRelation.cpp
167–175

I think a better way to write this loop would be to write it in reverse. Swapping each non-div with the offset + numDivisions and incrementing numDivisions after each iteration.

167–175

Also, please write some comments explaining what this does. I find this somewhat non-trivial to understand.

1172–1174

Not for this patch, but I do not like that we compute the division representation each time we want it. This computation is very expensive. We should instead be storing these divisions separately with each set.

arjunp added inline comments.Jun 15 2022, 2:42 PM
mlir/lib/Analysis/Presburger/IntegerRelation.cpp
167–175

numDivisions does not get incremented on every iteration in that implementation. i is only incremented in some cases as well, same as here. Why are you saying that way is better?

Groverkss added inline comments.Jun 15 2022, 11:52 PM
mlir/lib/Analysis/Presburger/IntegerRelation.cpp
167–175

Sorry for being unclear. I was just suggesting that I think it is more natural to write this loop in reverse. I find that loops working with divisions are easier to write when written in reverse. But since you are not using the division representation, this should be either way.

arjunp updated this revision to Diff 438144.Jun 18 2022, 1:03 PM
arjunp marked 3 inline comments as done.

Fix bug where the resulting set had a space incompatible with the space of the original set.

Also addressed Kunwar's comments.

arjunp updated this revision to Diff 438145.Jun 18 2022, 1:16 PM

Fix doc and add another test.

Groverkss added inline comments.Jun 18 2022, 3:32 PM
mlir/include/mlir/Analysis/Presburger/IntegerRelation.h
101–104

This is very confusing and I cannot understand what this does. Can you please write it more clearly?

747–749

I think writing about union of convex disjuncts is enough.

mlir/include/mlir/Analysis/Presburger/PresburgerRelation.h
60

I would rather call them "extra" ids.

63
mlir/unittests/Analysis/Presburger/PresburgerSetTest.cpp
764

I think you should put a newline after this.

arjunp updated this revision to Diff 438548.Jun 20 2022, 9:31 PM
arjunp marked 6 inline comments as done.

Address kunwar's comments

This revision is now accepted and ready to land.Jun 22 2022, 10:05 AM
This revision was landed with ongoing or failed builds.Jun 25 2022, 6:23 AM
This revision was automatically updated to reflect the committed changes.