This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] The return type in the `computeSingleVarRepr` function is modified to include equality expressions.
ClosedPublic

Authored by pashu123 on Jan 16 2022, 7:52 AM.

Details

Summary

Earlier computeSingleVarRepr was returning a pair of upper bound and
lower bound indices of the inequality contraints that can be expressed
as a floordiv of an affine function. The equality expression can also be
expressed as a floordiv but contains only one index and hence the LocalRepr
class is introduced to facilitate this.

Diff Detail

Event Timeline

pashu123 created this revision.Jan 16 2022, 7:52 AM
pashu123 requested review of this revision.Jan 16 2022, 7:52 AM
pashu123 updated this revision to Diff 400387.Jan 16 2022, 8:18 AM

Clang format.

Groverkss added inline comments.Jan 16 2022, 8:59 AM
mlir/lib/Analysis/AffineStructures.cpp
859–860 ↗(On Diff #400383)

Until support for equalities is added I would recommend returning false here in case of equality also.

mlir/lib/Analysis/Presburger/IntegerPolyhedron.cpp
839

You can add using namespace presburger_utils in this file and use ReprKind directly.

mlir/lib/Analysis/Presburger/PresburgerSet.cpp
224–225

The name "maybePair" does not make sense now. Please change it to repr or something similar.

mlir/lib/Analysis/Presburger/Utils.cpp
148

I would recommend add using namespace presburge_utils in this file and using LocalRepr directly.

Since LocalRepr is nullable, a better name for it might be MaybeLocalRepr.

pashu123 updated this revision to Diff 400866.Jan 18 2022, 9:01 AM

Addressing comments.

pashu123 updated this revision to Diff 401336.Jan 19 2022, 10:50 AM
pashu123 marked 2 inline comments as done.

Changing LocalRepr to MaybeLocalRepr.

pashu123 marked 2 inline comments as done.Jan 19 2022, 10:51 AM
pashu123 added inline comments.
mlir/lib/Analysis/Presburger/PresburgerSet.cpp
224–225

Since we are asserting for inequality any way below, I think maybeInequality makes sense.

pashu123 updated this revision to Diff 401359.Jan 19 2022, 11:55 AM
pashu123 marked an inline comment as done.

Minor change.

This revision is now accepted and ready to land.Jan 20 2022, 2:58 AM
mehdi_amini added inline comments.Jan 24 2022, 5:25 PM
mlir/lib/Analysis/Presburger/Utils.cpp
157

It's not clear that this is guaranteed to be initialized before the return statement.
I think you can just add braces here: MaybeLocalRepr repr{};

bondhugula added a comment.EditedJan 24 2022, 5:44 PM

Please rewrite the commit title to use active: eg:
[MLIR] Update return type for computeSingleVarRepr` to include equalities`
(much more compact)
Edit: Looks like this was already merged.

Please rewrite the commit title to use active: eg:
[MLIR] Update return type for computeSingleVarRepr` to include equalities`
(much more compact)
Edit: Looks like this was already merged.

Next time I will take care of this. Thanks.

mlir/lib/Analysis/Presburger/Utils.cpp
157

@mehdi_amini I have the required changes in this patch: https://reviews.llvm.org/D117959