This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Move presburger math from FlatAffineConstraints to Presburger directory
AbandonedPublic

Authored by Groverkss on Dec 26 2021, 5:30 AM.

Details

Summary

This patch moves remaining math functionality in FlatAffineConstraints to
IntegerPolyhedron. This patch also moves LinearTransform to Presburger/
directory and renames FlatAffineConstraints to IntegerPolyhedron in
Presburger/Simplex and Presburger/Utils.

This patch is a continuation of https://reviews.llvm.org/D114674.

Diff Detail

Event Timeline

Groverkss created this revision.Dec 26 2021, 5:30 AM
Groverkss requested review of this revision.Dec 26 2021, 5:30 AM
Groverkss edited the summary of this revision. (Show Details)Dec 26 2021, 5:31 AM

The only non-trivial change in this patch that would require you to look at the implementation is IntegerPolyhedron::print. FlatAffineValueConstraints prints information about SSA Values, this information is now printed by a virtual function IntegerPolyhedron::printSpace which FlatAffineConstraints overrides to include the SSA Value information.

The rest of the patch is only moving and renamed to the Presburger directory.

Groverkss updated this revision to Diff 396223.Dec 26 2021, 6:18 AM
  • Fix clang-format error

I would suggest just doing the move of functions from FAC to IP in this patch, and doing the other things like using IP in PresbugerSet and LinearTransform and moving LinearTransform in different patches.

Also, in the summary I would change "renames FlatAffineConstraints to IntegerPolyhedron" to "uses "IntegerPolyhedron instead of FlatAffineConstraints".

mlir/include/mlir/Analysis/AffineStructures.h
115

Why is this needed exactly? Might be useful to write a comment.

131

nit: seems to be moved unnecessarily

mlir/include/mlir/Analysis/PresburgerSet.h
120
mlir/lib/Analysis/AffineStructures.cpp
1344–1347

Can't you dyn_cast the IntegerPolyhedron directly to a FlatAffineValueConstraints?

1351

Isn't this already a FlatAffineConstraints *?

Is the static cast to other needed here?

1356–1362

Again is otherFAC needed to be used here?

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

I think variable names like fac should be changed throughout.

mlir/lib/Analysis/PresburgerSet.cpp
29
424
Groverkss abandoned this revision.Dec 26 2021, 11:09 AM

Thank you for the review. I'll split this patch up into smaller patches to make it easier to review and take the comments in consideration.