This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] Add hermite normal form computation to Matrix
ClosedPublic

Authored by Groverkss on Sep 8 2022, 11:14 AM.

Details

Summary

This patch adds hermite normal form computation to Matrix. Part of this algorithm
lived in LinearTransform, being used for compuing column echelon form. This
patch moves the implementation to Matrix::hermiteNormalForm and generalises it
to compute the hermite normal form.

Diff Detail

Event Timeline

Groverkss created this revision.Sep 8 2022, 11:14 AM
Groverkss requested review of this revision.Sep 8 2022, 11:14 AM

Thanks @Groverkss ! Any performance numbers to share?

Groverkss added a comment.EditedSep 9 2022, 3:11 AM

Thanks @Groverkss ! Any performance numbers to share?

I would assume that the computation works fast and should not bottleneck anything assuming that the number of columns is not too huge (which is generally true for polyhedra).

I still tried to run some experiments:

For a 10x10 matrix with almost 4 non-zero elements per row (generally polyhedra 2-3 non-zero elements per row in inequality/equality matrix):

{
  Matrix mat = makeMatrix(10, 10,
                          {
                              {1, 0, 0, 0, 1, 0, 0, 0, 0, 0},
                              {0, 1, 0, 0, 0, 0, 0, 0, 0, 1},
                              {0, 0, 0, 3, 0, 0, 4, 0, 0, -1},
                              {0, 0, 1, 0, 0, -1, 0, 0, 0, 0},
                              {0, 0, 0, -1, 0, 0, 0, 0, 0, -1},
                              {0, 0, 0, 1, 0, 0, 0, 0, 2, 10},
                              {0, 0, 0, 0, 3, 0, 0, 2, 0, 0},
                              {0, 1, 0, 0, 0, 0, 5, 0, 0, 0},
                              {0, 0, 0, 1, 0, 1, 0, 0, 1, 1},
                              {0, 0, 1, 0, 0, 1, 0, 1, 0, 0},
                          });
  auto [h, u] = mat.hermiteNormalForm();
}

Running this computation 100 times takes 2ms in a debug build. I would expect it to be faster in a release build.

I also tried the computation for dense 10x10 matrices, but while it runs fast, it is incorrect due to overflows (which should be fixed by https://reviews.llvm.org/D129510)

Are there any specific numbers that you are looking for? Or are these fine?

arjunp added inline comments.Sep 9 2022, 5:10 AM
mlir/include/mlir/Analysis/Presburger/Matrix.h
166
168

use std::pair.

168
mlir/lib/Analysis/Presburger/LinearTransform.cpp
20–21
24
mlir/lib/Analysis/Presburger/Matrix.cpp
348
350–364

It would be better to just support negative targetCol in modEntryColumnOperation and use that

mlir/unittests/Analysis/Presburger/MatrixTest.cpp
201
229–239

Please write a function to do this

Groverkss updated this revision to Diff 459095.Sep 9 2022, 9:28 AM
Groverkss marked 9 inline comments as done.

Address Arjun's comments

Groverkss added inline comments.Sep 9 2022, 9:30 AM
mlir/lib/Analysis/Presburger/Matrix.cpp
348

This is not required because the scale is always assigned a value before use.

Address more comments.

arjunp added inline comments.Sep 14 2022, 7:09 AM
mlir/lib/Analysis/Presburger/Matrix.cpp
264–271

ratio should just be floorDiv(m(row, targetCol), m(row, sourceCol)) right? (with scale =-ratio below)

Groverkss updated this revision to Diff 460087.Sep 14 2022, 7:40 AM
Groverkss marked an inline comment as done.

Address Arjun's comment

arjunp accepted this revision.Sep 14 2022, 7:53 AM
arjunp added inline comments.
mlir/lib/Analysis/Presburger/Matrix.cpp
263

Maybe add some documentation explaining this.

This revision is now accepted and ready to land.Sep 14 2022, 7:53 AM