This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][FlatAffineConstraints][NFC] Move some static functions to be available to Presburger/
ClosedPublic

Authored by Groverkss on Dec 16 2021, 3:47 AM.

Details

Summary

This patch moves some static functions from AffineStructures.cpp to
Presburger/Utils.cpp and some to be private members of FlatAffineConstraints
(which will later be moved to IntegerPolyhedron) to allow for a smoother
transition for moving FlatAffineConstraints math functionality to
Presburger/IntegerPolyhedron.

This patch is part of a series of patches for moving math functionality to
Presburger directory.

Diff Detail

Event Timeline

Groverkss created this revision.Dec 16 2021, 3:47 AM
Groverkss requested review of this revision.Dec 16 2021, 3:47 AM
arjunp accepted this revision.Dec 16 2021, 4:39 AM

Seems good to me. I would wait for a few days to land in case @bondhugula or others have any comments.

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

I think the C++ is only used for headers, not for .cpp files which are anyway known to be C++.

This revision is now accepted and ready to land.Dec 16 2021, 4:39 AM
bondhugula requested changes to this revision.Dec 16 2021, 6:51 AM
bondhugula added inline comments.
mlir/include/mlir/Analysis/Presburger/Utils.h
24

I don't think a function with such a name should be exposed on mlir::. In fact, this can just be a method on FlatAffineCosntraints.

36

Likewise -- these shouldn't be exposed on mlir::.

This revision now requires changes to proceed.Dec 16 2021, 6:51 AM
Groverkss marked 3 inline comments as done.

Address Uday and Arjun's comments

Groverkss retitled this revision from [MLIR][NFC] Move some static functions from AffineStructures.cpp to Presburger/Utils.cpp to [MLIR][FlatAffineConstraints][NFC] Move some static functions to be available to Presburger/.Dec 16 2021, 12:30 PM
Groverkss edited the summary of this revision. (Show Details)
bondhugula added inline comments.Dec 20 2021, 8:39 AM
mlir/include/mlir/Analysis/Presburger/Utils.h
17

I don't think you need this header. Please use forward declarations instead -- unnecessary header includes impact build time negatively.

20

namespace uses snake case I think -- can you check other examples in the code base?

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

foundRepr should have been an ArrayRef. I understand you are just moving but this should be fixed.

bondhugula accepted this revision.Dec 20 2021, 8:39 AM
This revision is now accepted and ready to land.Dec 20 2021, 8:39 AM
Groverkss marked 3 inline comments as done.
  • Address Uday's comments
Groverkss reopened this revision.Dec 24 2021, 11:38 AM

Reverted because this change broke the mlir-nvidia build bot (but surprisingly passed the builds) https://lab.llvm.org/buildbot/#/builders/61/builds/19184

This revision is now accepted and ready to land.Dec 24 2021, 11:38 AM
  • Remove dependency of Presburger/Utils on FlatAffineConstraints
Groverkss requested review of this revision.Dec 24 2021, 12:01 PM

Removed dependency on FlatAffineConstraints for Presburger/Utils. This required moving a function from FlatAffineConstraints to IntegerPolyhedron. This will not affect any design since the next patch I plan to send would have moved it anyway.

Reverted because this change broke the mlir-nvidia build bot (but surprisingly passed the builds) https://lab.llvm.org/buildbot/#/builders/61/builds/19184

Thanks for the quick revert! Please think about mentioning the revert reason in the revert commit message itself.

Reverted because this change broke the mlir-nvidia build bot (but surprisingly passed the builds) https://lab.llvm.org/buildbot/#/builders/61/builds/19184

Thanks for the quick revert! Please think about mentioning the revert reason in the revert commit message itself.

Sorry. Will keep that in mind next time :)

arjunp accepted this revision.Dec 25 2021, 7:43 AM

LGTM.

This revision is now accepted and ready to land.Dec 25 2021, 7:43 AM