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
Details
Diff Detail
Event Timeline
Template Matrix to work with MPInt and Fraction
Add printing capability to Fraction
All previous uses of Matrix are replaced with Matrix<MPInt>
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.)
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 | |
| 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? | |
| 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.") | |
| 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)}, ...}) | |
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.
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.