This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] rewrite AffineStructures and Presburger tests to use the parser
ClosedPublic

Authored by Dinistro on Dec 17 2021, 12:30 AM.

Details

Summary

This commit rewrites most existing unittests involving FlatAffineConstraints to use the parsing utility. This helps to make the tests more understandable.

Diff Detail

Event Timeline

Dinistro created this revision.Dec 17 2021, 12:30 AM
Dinistro published this revision for review.Dec 17 2021, 2:45 AM
Dinistro added inline comments.
mlir/unittests/Analysis/AffineStructuresTest.cpp
163

Do you guys think that it makes sense to write these down as one constraint per line?

519

This is not actually related to this patch, should I remove these changes?

574

The parser can only introduce divisions if they actually show up in a constraint. Should I add a constraint that introduces these divisions or should we just drop that TODO?

mlir/unittests/Analysis/PresburgerSetTest.cpp
348–349

Alternatively, I could add a function that just takes a list of StringRefs which are all parsed and unioned.

Thanks for the patch!

mlir/unittests/Analysis/AffineStructuresTest.cpp
163

Not sure, but at the very least it would be helpful to not break any single constraint across multiple lines.

519

I guess ideally yes. Not sure how much it matters.

574

I think you can use the parser with some dummy constraint here. We should be careful that it doesn't change the representation. (Adding a tighter constraint can change the returned numerator's constant term.) I think constraining each div to be greater than equal to zero would be good. Not sure if this works for nested divisions though; if there are any such cases we can leave them as is for now.

mlir/unittests/Analysis/PresburgerSetTest.cpp
348–349

Yeah, I think that would be helpful.

arjunp retitled this revision from [MLIR] rewrite FAC tests to use the parsr to [MLIR] rewrite FAC tests to use the parser.
Dinistro updated this revision to Diff 395090.Dec 17 2021, 3:38 AM

Change strings to not break constraints over multiple lines

Dinistro marked an inline comment as done.Dec 17 2021, 3:39 AM
Dinistro added inline comments.Dec 17 2021, 8:06 AM
mlir/unittests/Analysis/AffineStructuresTest.cpp
574

This does unfortunately not work as we want it to.

The internal representation of fac looks as follows:

Constraints (1 dims, 0 symbols, 2 locals), (4 constraints)
(None None None  const)
1 -10 0 4 >= 0
-1 10 0 5 >= 0
1 0 -10 100 >= 0
-1 0 10 -91 >= 0

But parsing, for example, "(x) : ((x + 4) floordiv 10 == 0, (x + 100) floordiv 10 == 0)" yields the following representation:

Constraints (1 dims, 0 symbols, 2 locals), (6 constraints)
(None None None  const)
0 1 0 0 = 0
0 0 1 10 = 0
1 -10 0 4 >= 0
-1 10 0 5 >= 0
1 0 -10 0 >= 0
-1 0 10 9 >= 0

I'm not sure what changes the representation of the divisions, but this does break the test.

Did I do something wrong or is this indeed a simplification the parser does?

Dinistro updated this revision to Diff 395141.Dec 17 2021, 8:19 AM

Add example of a helper to parse a list of FAC strings.

Dinistro added inline comments.Dec 17 2021, 8:20 AM
mlir/unittests/Analysis/PresburgerSetTest.cpp
348–349

I added an example of such a helper function. Is this what you expected?

arjunp added inline comments.Dec 17 2021, 10:18 AM
mlir/unittests/Analysis/AffineStructuresTest.cpp
574

Yes, I think this is indeed something done by the parser. Maybe @Groverkss can confirm, but I don't think we have such simplifications anywhere else yet. I would just leave the division stuff out for now then.

In the future we will implement such simplifications internally as well, and we would want to keep these tests as is so that we can test that functionality I guess.

mlir/unittests/Analysis/PresburgerSetTest.cpp
348–349

Yes, exactly.

Dinistro updated this revision to Diff 395273.Dec 18 2021, 1:49 AM

Use the Presburger parsing helper function for all PresburgerSet creations.

Dinistro marked 2 inline comments as done.Dec 18 2021, 1:50 AM

LGTM. I would wait for others to accept in case they have any remaining concerns.

mlir/unittests/Analysis/AffineStructuresTest.cpp
574

We do not have such simplifications. It's the parser that does these.

I think it's better to not use the parser here. I don't think the parser will put the divisions in the order we want them to and it may become more confusing. You can drop the TODO according to me.

Dinistro updated this revision to Diff 395283.Dec 18 2021, 4:51 AM

Drop outdated TODO

Dinistro marked 3 inline comments as done.Dec 18 2021, 4:52 AM
arjunp accepted this revision.Dec 19 2021, 4:39 AM

LGTM, please fix the minor comment issues below.

mlir/unittests/Analysis/AffineStructuresTest.cpp
182–188

Please add back the comment above.

// Same thing with some spurious extra dimensions equated to constants.
506–508

These comments don't make sense anymore. I would replace the [0], [1] etc. by the actual constraints.

mlir/unittests/Analysis/PresburgerSetTest.cpp
480–481

This outdated comment can be removed now.

This revision is now accepted and ready to land.Dec 19 2021, 4:39 AM
Dinistro updated this revision to Diff 395320.Dec 19 2021, 5:25 AM
Dinistro marked 3 inline comments as done.

Address remaining comments.

Thanks a lot for this patch!

arjunp retitled this revision from [MLIR] rewrite FAC tests to use the parser to [MLIR] rewrite AffineStructures and Presburger tests to use the parser.Dec 19 2021, 5:55 AM

Sorry: I had to revert because this broke the ASAN/UBSAN CI (see the trace in the revert commit message(

arjunp added inline comments.Dec 20 2021, 2:26 AM
mlir/unittests/Analysis/AffineStructuresTest.cpp
222

The mutated test case is too big to be computed with 64-bits, I believe that's why I initially chose this number of digits for this test case. I can't believe I manually checked the patch and still missed this

Dinistro reopened this revision.Dec 20 2021, 6:12 AM
This revision is now accepted and ready to land.Dec 20 2021, 6:12 AM
Dinistro updated this revision to Diff 395430.Dec 20 2021, 6:13 AM
  • Fix coefficient that caused UB.
Dinistro marked an inline comment as done.Dec 20 2021, 6:14 AM

Im sorry for this problem.
Thanks for pointing out the issue, should be fixed now.