Page MenuHomePhabricator

[MLIR][Presburger] Refactor duplicate division merging to Utils
ClosedPublic

Authored by Groverkss on Jan 23 2022, 11:38 AM.

Details

Summary

This patch moves merging of duplicate divisions to presburger utility
functions. This is required to support division merging in structures other
than IntegerPolyhedron.

Diff Detail

Event Timeline

Groverkss created this revision.Jan 23 2022, 11:38 AM
Groverkss requested review of this revision.Jan 23 2022, 11:38 AM
arjunp requested changes to this revision.Jan 23 2022, 11:58 AM
arjunp added inline comments.
mlir/include/mlir/Analysis/Presburger/Utils.h
60–61

I think it'll be helpful if you mention clearly that division j thus gets eliminated, and its coefficients get added to i.

61
68

Why is this return value needed?

68

I think you can specify parameter names here

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

can mention parameter names here too

This revision now requires changes to proceed.Jan 23 2022, 11:58 AM
Groverkss marked 5 inline comments as done.Jan 23 2022, 12:08 PM
Groverkss added inline comments.
mlir/include/mlir/Analysis/Presburger/Utils.h
68

This is in case some merge does not want to merge divisions belonging to the same set, which I think we wanted to implement at some point.

Groverkss marked an inline comment as done.
  • Address arjun's comments
arjunp added inline comments.Jan 23 2022, 12:18 PM
mlir/include/mlir/Analysis/Presburger/Utils.h
68

I think that can be resolved by just having a separate function to merge within a set and calling that earlier, and I prefer that solution.

arjunp added inline comments.Jan 23 2022, 12:24 PM
mlir/include/mlir/Analysis/Presburger/Utils.h
69
Groverkss marked an inline comment as done.
  • Use llvm::function_ref instead of std::function &
mlir/include/mlir/Analysis/Presburger/Utils.h
68

I do not completely understand the way you propose. Could you elaborate more?

I think the returning a bool way makes it more general and its a small addition only.

arjunp accepted this revision.Jan 24 2022, 12:03 AM
arjunp added inline comments.
mlir/include/mlir/Analysis/Presburger/Utils.h
68

Sorry, I see that was unclear. What I meant is just write a separate simplify function in IntegerPolyhedron to just simplify divisions within the set. If we call that initially before we run whatever algorithm is using merging, then we know that internal simplifications won't be a problem after that.

I'm fine with keeping the bool return as well, but I'm not sure this generality is needed.

This revision is now accepted and ready to land.Jan 24 2022, 12:03 AM