Page MenuHomePhabricator

[MLIR] Extract division representation from equality expressions.
ClosedPublic

Authored by pashu123 on Jan 22 2022, 10:53 AM.

Details

Summary

Extract the division representation from equality constraints.
For example:

32*k == 16*i + j - 31                 <-- k is the localVariable
expr = 16*i + j - 31, divisor = 32
k = (16*i + j - 32) floordiv 32

The dividend of the division is set to [16, 1, -32] and the divisor is set
to 32.

Diff Detail

Event Timeline

pashu123 created this revision.Jan 22 2022, 10:53 AM
pashu123 requested review of this revision.Jan 22 2022, 10:53 AM
pashu123 updated this revision to Diff 402241.Jan 22 2022, 11:05 AM

Minor comments.

pashu123 updated this revision to Diff 402287.Jan 22 2022, 9:42 PM

Updating comments.

pashu123 updated this revision to Diff 402298.Jan 23 2022, 1:00 AM

Clang format.

Groverkss added inline comments.Jan 23 2022, 3:55 AM
mlir/include/mlir/Analysis/Presburger/IntegerPolyhedron.h
229

Why is this default value removed? If you do decide to remove it, please specify how to make equalities optional (i.e. by passing a nullptr).

mlir/lib/Analysis/Presburger/Utils.cpp
180–182
186

I don't think you need to pass this by reference.

mlir/unittests/Analysis/Presburger/IntegerPolyhedronTest.cpp
739–774

Could you add some tests where there are both inequalities and equalities and divisions are extracted from both?

arjunp added inline comments.Jan 23 2022, 10:16 AM
mlir/lib/Analysis/Presburger/IntegerPolyhedron.cpp
823

Maybe not for this patch, but the std::vector can be changed to SmallVector now.

858–863
mlir/lib/Analysis/Presburger/Utils.cpp
163

I would suggest either writing a comment that the equality must involve the pos-th variable and assert temp_div != 0, or return failure if temp_div == 0.

pashu123 updated this revision to Diff 403300.Jan 26 2022, 9:07 AM

Addressing arjun's and Kunwar's comments.

pashu123 updated this revision to Diff 403301.Jan 26 2022, 9:09 AM

minor comments.

pashu123 updated this revision to Diff 403303.Jan 26 2022, 9:12 AM
pashu123 marked an inline comment as done.

Addressing comments.

pashu123 updated this revision to Diff 403309.Jan 26 2022, 9:14 AM
pashu123 marked an inline comment as done.

Removing unneccesary debugs.

pashu123 updated this revision to Diff 403310.Jan 26 2022, 9:15 AM

Updating comments.

pashu123 marked 3 inline comments as done.Jan 26 2022, 9:16 AM
pashu123 added inline comments.
mlir/lib/Analysis/Presburger/IntegerPolyhedron.cpp
823

Indeed, we can do that in the next patch.

858–863

I missed it. This is very elegant. Thanks.

pashu123 marked an inline comment as done.Jan 26 2022, 11:07 AM
pashu123 edited the summary of this revision. (Show Details)
Groverkss added a comment.EditedJan 26 2022, 10:22 PM
This comment has been deleted.
mlir/lib/Analysis/Presburger/IntegerPolyhedron.cpp
823

Please update the docs for getLocalReprs in header file to reflect the new return type MaybeLocalRepr and that it supports not only inequalities but also equalities.

mlir/lib/Analysis/Presburger/Utils.cpp
169–170
210–215

Please update these docs to reflect the return type. Same in the header file.

pashu123 updated this revision to Diff 403659.Jan 27 2022, 7:59 AM

Updating comments.

pashu123 marked 2 inline comments as done.Jan 27 2022, 8:00 AM
pashu123 updated this revision to Diff 403660.Jan 27 2022, 8:02 AM

Clang format warnings.

arjunp added inline comments.Jan 28 2022, 8:06 AM
mlir/lib/Transforms/Utils/LoopUtils.cpp
3293 ↗(On Diff #403660)

Why is this change required?

pashu123 updated this revision to Diff 404034.Jan 28 2022, 8:16 AM

removing unneccesary changes.

pashu123 marked an inline comment as done.Jan 28 2022, 8:16 AM
pashu123 added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
3293 ↗(On Diff #403660)

Reverting, this is not required.

pashu123 marked 2 inline comments as done.Jan 28 2022, 9:00 AM
Groverkss added inline comments.Jan 28 2022, 4:52 PM
mlir/lib/Analysis/Presburger/Utils.cpp
188

I don't see any reason for this to be const.

pashu123 added inline comments.Jan 28 2022, 10:51 PM
mlir/lib/Analysis/Presburger/Utils.cpp
188

We don't want pos to be changed by the function and hence making it const makes sense here. There are many benefits associated: const tells the compiler that a variable or method is immutable. This helps the compiler optimize the code and helps the developer know if a function has a side effect. Also, using const & prevents the compiler from copying data unnecessarily.

pashu123 added inline comments.Jan 29 2022, 12:08 AM
mlir/lib/Analysis/Presburger/Utils.cpp
188

I think pass by reference and const makes more sense.

pashu123 updated this revision to Diff 404529.Jan 31 2022, 7:43 AM

Removing const.

pashu123 marked an inline comment as done.Jan 31 2022, 7:44 AM
Groverkss accepted this revision.Feb 1 2022, 2:08 AM

LGTM. Just a minor comment.

mlir/lib/Analysis/Presburger/Utils.cpp
169–170

I think you missed this comment.

This revision is now accepted and ready to land.Feb 1 2022, 2:08 AM
pashu123 updated this revision to Diff 404863.Feb 1 2022, 2:53 AM
pashu123 marked an inline comment as done.

Minor comments.

pashu123 marked an inline comment as done.Feb 1 2022, 2:53 AM
This revision was landed with ongoing or failed builds.Feb 1 2022, 2:54 AM
This revision was automatically updated to reflect the committed changes.
arjunp added inline comments.Feb 1 2022, 4:05 AM
mlir/lib/Analysis/Presburger/Utils.cpp
163

Oh, I think it should be called tempDiv.