This results in significant deduplication of code. This patch is not expected to change any functionality, it's just some simplification in preparation for future work. Also slightly simplified some code that was being touched anyway and added some unit tests for some functions that were touched.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This patch is not expected to change any functionality, it's just some simplification in preparation for future work.
mlir/lib/Analysis/AffineStructures.cpp | ||
---|---|---|
146–147 | Is there a reason the default copy constructor was not used? |
Thanks @arjunp Could you please add this line to the commit summary and add an additional line there on this patch itself?
This looks good in general. Some minor comments below.
mlir/include/mlir/Analysis/AffineStructures.h | ||
---|---|---|
70–76 | Thanks for cleaning this up - this looks unrelated to the patch. This should at least be mentioned in the summary. "FlatAffineConstraints cleanup"? | |
88–93 | Thanks for removing duplication here. Please also use the "/*....=*/" form wherever constant integers, etc. are being passed - for readability. I think numReservedRows/Cols was earlier uninitialized? | |
mlir/include/mlir/Analysis/Presburger/Matrix.h | ||
65–66 | It would be good to have a doc comment for these. | |
130 | Doc comment please. | |
130–132 | nReservedColumns needs a doc comment at least. | |
mlir/lib/Analysis/Presburger/Matrix.cpp | ||
96 | llvm math has this function: https://llvm.org/doxygen/MathExtras_8h_source.html | |
mlir/unittests/Analysis/Presburger/MatrixTest.cpp | ||
94–115 | A bit weird that these tests are under Analysis/Presburger. Is there a better place for them? |
mlir/include/mlir/Analysis/Presburger/Matrix.h | ||
---|---|---|
67–101 | All doc comments should be triple ///. |
Thanks for the quick review!
mlir/include/mlir/Analysis/AffineStructures.h | ||
---|---|---|
70–76 | My primary reason to touch this code was to fix the usage of inequalities.reserve and the reference to nReservedColumns in accordance with the change to Matrix. I did slightly simplify the code since I was touching it anyway; I've mentioned this in the summary now. | |
mlir/unittests/Analysis/Presburger/MatrixTest.cpp | ||
94–115 | Not sure where else to put it since Matrix is under Analysis/Presburger too at the moment. I'm happy to have them both moved in another patch if there's a better place for them. |
Thanks for cleaning this up - this looks unrelated to the patch. This should at least be mentioned in the summary. "FlatAffineConstraints cleanup"?