This is an archive of the discontinued LLVM Phabricator instance.

Template Matrix to allow both MPInt and Fraction
Needs RevisionPublic

Authored by Abhinav271828 on Aug 2 2023, 8:05 AM.

Details

Reviewers
arjunp
Groverkss
Summary

Template Matrix to Matrix<T> (for MPInt and Fraction) with explicit instantiation
Duplicate makeMatrix to makeIntMatrix and makeFracMatrix
Implement arithmetic operations for Fraction for compatibility

Diff Detail

Event Timeline

Abhinav271828 created this revision.Aug 2 2023, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 8:05 AM
Abhinav271828 requested review of this revision.Aug 2 2023, 8:05 AM

Template Matrix to work with MPInt and Fraction
Add printing capability to Fraction
All previous uses of Matrix are replaced with Matrix<MPInt>

Abhinav271828 retitled this revision from Shift inverse from LinearTransform to MatrixF to Template Matrix to allow both MPInt and Fraction.
Abhinav271828 edited the summary of this revision. (Show Details)

Looks like you forgot some commits. Could you check?

Abhinav271828 edited the summary of this revision. (Show Details)

Looks like you forgot some commits. Could you check?

Changed the diff, does it look all right now?

I suggest breaking up this patch further. Let's keep this patch to only templating the Matrix class. Adding new functionality can be done in a separate patch. How does inverse work when it's a matrix of integers?

Abhinav271828 edited the summary of this revision. (Show Details)
Abhinav271828 added a comment.EditedAug 29 2023, 7:09 AM

I suggest breaking up this patch further. Let's keep this patch to only templating the Matrix class. Adding new functionality can be done in a separate patch. How does inverse work when it's a matrix of integers?

Done! Only the templating is part of the patch now.
Inverse is only defined for rational matrices as of now.

Templated C++ classes usually need to have all their implementations in the header but this increases compile time. However we only need Matrix<MPInt> and Matrix<Fraction>.

I think the best option here is to just rename Matrix<MPInt> to IntMatrix (using IntMatrix = detail::Matrix<MPInt>), hide the templated matrix class in a namespace detail, and document that it's not intended for general use. You then have to explicitly instantiate Matrix<MPInt> in the .cpp. (forget about Matrix<Fraction> for now.) Add this line to the end of the .cpp

template class Matix<MPInt>;

(https://stackoverflow.com/questions/2351148/explicit-template-instantiation-when-is-it-used)

(I also considered putting static_assert that the type is a supported one, but this seems cleaner to me.)

I suggest breaking up this patch further. Let's keep this patch to only templating the Matrix class. Adding new functionality can be done in a separate patch. How does inverse work when it's a matrix of integers?

Done! Only the templating is part of the patch now.
Inverse is only defined for rational matrices as of now.

It's not a good idea to list functions in the header that then surprisingly turn out to not have been implemented. I would probably just move it out into a free function for that patch. The Matrix class atm is supposed to be a 2D array anyway, not a linear transform class. We can discuss whether that's a good idea but it's beyond the scope of these patches.

mlir/lib/Analysis/Presburger/Matrix.cpp
240

why was this changed?

252

same here

mlir/unittests/Analysis/Presburger/MatrixTest.cpp
212–213

Please rename makeMatrix to makeIntMatrix and use that instead of templating makeMatrix and adding constructors hre. The current solution is difficult to read

Abhinav271828 added inline comments.Aug 30 2023, 7:48 AM
mlir/lib/Analysis/Presburger/Matrix.cpp
240

Fraction doesn't have += implemented, so the compilation failed for that instantiation.

mlir/unittests/Analysis/Presburger/MatrixTest.cpp
212–213

What about when we need to use makeMatrix for fractions? Should I duplicated makeMatrix to makeIntMatrix and makeFracMatrix?

Abhinav271828 edited the summary of this revision. (Show Details)
arjunp added inline comments.Sep 1 2023, 11:08 AM
mlir/lib/Analysis/Presburger/Matrix.cpp
380–387

The braces should be in the same line and namespace contents shouldn't be indented

mlir/unittests/Analysis/Presburger/Utils.h
43–45

Not sure if this will work, you might need an std::pair to make the brace initialization actually work

arjunp added inline comments.Sep 1 2023, 11:24 AM
mlir/include/mlir/Analysis/Presburger/Matrix.h
33

Please mention here that this is only intended for use with Fraction and MPInt. And add a static assert to the top of the class checking that T is one of these types.

static_assert(std::is_same_v<T,MPInt> || std::is_same_v<T,Fraction>, "T must be MPInt or Fraction.")

Please write full sentences in the patch summary.

Abhinav271828 marked an inline comment as done.Sep 4 2023, 5:38 AM
Abhinav271828 added inline comments.
mlir/unittests/Analysis/Presburger/Utils.h
43–45

The tests use the constructor itself, e.g. makeFracMatrix(2, 2, {{Fraction(1, 1), Fraction(0, 1)}, ...})

Groverkss requested changes to this revision.Sep 5 2023, 12:21 AM

This patch was sent as a pull request instead: https://github.com/llvm/llvm-project/pull/65272. Please abandon this revision to reduce the number of open revisions we have.

This revision now requires changes to proceed.Sep 5 2023, 12:21 AM