This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Presburger] Improve unittest parsing
ClosedPublic

Authored by Groverkss on Sep 10 2022, 6:02 PM.

Details

Summary

This patch adds better functions for parsing MultiAffineFunctions and
PWMAFunctions in Presburger unittests.

A PWMAFunction can now be parsed as:

PWMAFunction result = parsePWMAF({
    {"(x, y) : (x >= 10, x <= 20, y >= 1)", "(x, y) -> (x + y)"},
    {"(x, y) : (x >= 21)", "(x, y) -> (x + y)"},
    {"(x, y) : (x <= 9)", "(x, y) -> (x - y)"},
    {"(x, y) : (x >= 10, x <= 20, y <= 0)", "(x, y) -> (x - y)"},
});

which is much more readable than the old format since the output can be
described as an AffineMap, instead of coefficients.

This patch also adds support for parsing divisions in MultiAffineFunctions
and PWMAFunctions which was previously not possible.

Diff Detail

Event Timeline

Groverkss created this revision.Sep 10 2022, 6:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
Groverkss requested review of this revision.Sep 10 2022, 6:02 PM
Groverkss edited the summary of this revision. (Show Details)
arjunp accepted this revision.Sep 14 2022, 8:30 AM
arjunp added inline comments.
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp
1817

This message does not seem very useful to the caller of getMultiAffineFunctionFromMap. Could you make it "map should only have locals with division representation"?

mlir/unittests/Analysis/Presburger/Parser.h
30
38
39
57–59

please either use llvm_unreachable or store the result in a variable and assert it to be success

65
85
This revision is now accepted and ready to land.Sep 14 2022, 8:30 AM
arjunp requested changes to this revision.Sep 14 2022, 8:32 AM
arjunp added inline comments.
mlir/lib/AsmParser/AffineParser.cpp
760–771

Can you assert that the affinemap/integerset was indeed the one that got parsed?

This revision now requires changes to proceed.Sep 14 2022, 8:32 AM
Groverkss marked 8 inline comments as done.Sep 14 2022, 9:07 AM
Groverkss added inline comments.
mlir/lib/Dialect/Affine/Analysis/AffineStructures.cpp
1817

This message is not for the caller. AffineMap should always have divisions. If this happens, there is a bug in affine map flattening.

Groverkss updated this revision to Diff 460130.Sep 14 2022, 9:08 AM
Groverkss marked an inline comment as done.

Address comments and rebase

arjunp added inline comments.Sep 15 2022, 3:05 AM
mlir/include/mlir/AsmParser/AsmParser.h
84

Outdated comment. Why was this argument removed?

Groverkss updated this revision to Diff 460353.Sep 15 2022, 3:36 AM
Groverkss marked an inline comment as done.

Address Arjun's comment and rebase

Groverkss added inline comments.Sep 15 2022, 3:43 AM
mlir/include/mlir/AsmParser/AsmParser.h
84

This was used to check if AffineMap/IntegerSet parsing failed on wrong inputs. This was already checked by AffineMap/IntegerSet tests. So it was just not required in unittests for parsing Presburger stuff.

arjunp added inline comments.Sep 15 2022, 3:53 AM
mlir/include/mlir/AsmParser/AsmParser.h
84

IIUC, previously when writing Presburger tests, helpful error messages were displayed to help debug syntax errors in the test sets, whereas now they will not be. Let's keep the error messages

Groverkss added inline comments.Sep 15 2022, 4:03 AM
mlir/include/mlir/AsmParser/AsmParser.h
84

They are still displayed by default. The flag was to disable those error messages

<mlir_parser_buffer>:1:17: error: non-affine expression: at least one of the multiply operands has to be either a constant or symbolic
(x, y) -> (0, x * y - 1)

I made an affine expression wrong in unit tests and got this error message, just to confirm.

arjunp accepted this revision.Sep 15 2022, 4:07 AM

Ah right, I should have read more carefully.

LGTM.

This revision is now accepted and ready to land.Sep 15 2022, 4:07 AM
This revision was automatically updated to reflect the committed changes.

I see some build issues with gcc8:

mlir/unittests/Analysis/Presburger/IntegerRelationTest.cpp:136:4: error: call of overloaded 'parsePWMAF(<brace-enclosed initializer list>)' is ambiguous
   });
    ^
mlir/unittests/Analysis/Presburger/Parser.h:64:1: note: candidate: 'mlir::presburger::PWMAFunction mlir::presburger::parsePWMAF(llvm::ArrayRef<std::pair<llvm::ArrayRef<llvm::StringRef>, llvm::StringRef> >)'
 parsePWMAF(ArrayRef<std::pair<ArrayRef<StringRef>, StringRef>> pieces) {
 ^~~~~~~~~~
mlir/unittests/Analysis/Presburger/Parser.h:84:1: note: candidate: 'mlir::presburger::PWMAFunction mlir::presburger::parsePWMAF(llvm::ArrayRef<std::pair<llvm::StringRef, llvm::StringRef> >)'
 parsePWMAF(ArrayRef<std::pair<StringRef, StringRef>> pieces) {
 ^~~~~~~~~~
Groverkss reopened this revision.Sep 15 2022, 10:54 AM
This revision is now accepted and ready to land.Sep 15 2022, 10:54 AM

Fix build issues with gcc8 and remove unused file

hokein added a subscriber: hokein.Sep 15 2022, 11:06 AM

I also reverted a related bazel change https://github.com/llvm/llvm-project/commit/10250c5a2a2ca6be683ff940d776648a2d5968e3, please include it when you reland this patch.

Fix bazel build

This revision was landed with ongoing or failed builds.Sep 15 2022, 11:29 AM
This revision was automatically updated to reflect the committed changes.
mlir/unittests/Dialect/Affine/Analysis/AffineStructuresParser.cpp