Page MenuHomePhabricator

Create the MLIR Reduce framework
ClosedPublic

Authored by msifontes on Mon, Jun 29, 1:25 PM.

Details

Summary

Create the framework and testing environment for MLIR Reduce - a tool
with the objective to reduce large test cases into smaller ones while
preserving their interesting behavior.

Implement the framework to parse the command line arguments, parse the
input MLIR test case into a module and call reduction passes on the MLIR module.

Implement the Tester class which allows the different reduction passes to test the
interesting behavior of the generated reduced variants of the test case and keep track
of the most reduced generated variant.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Updating D82803: Create the MLIR Reduce framework

Create include/mlir/Reducer directory and move Tester.h to it

msifontes updated this revision to Diff 274608.EditedTue, Jun 30, 1:51 PM

Modify testing script so that it is not OS specific

msifontes updated this revision to Diff 274660.Tue, Jun 30, 5:35 PM

Normalize path separators

msifontes updated this revision to Diff 274879.Wed, Jul 1, 11:14 AM

Remove mlir-opt from testing script

Remove mlir-opt call from the testing script to debug the Windows unit test that is currently failing.

rriddle added inline comments.Wed, Jul 1, 1:18 PM
mlir/include/mlir/Reducer/Tester.h
29

nit: Please use /// for top-level comments.

35

ArrayRef is a value-type and const by construction, so you can drop the explicit const &. Also please drop the llvm:: from ArrayRef, it is re-exported in the mlir namespace.

44

nit: Drop the std::move, it doesn't do anything differently from a normal assignment. Operations are non-owning smart pointers, there is no need to move them.

mlir/tools/mlir-reduce/Tester.cpp
34 ↗(On Diff #274879)

Why is the testCast pushed into the argument list twice?

44 ↗(On Diff #274879)

nit: return success(result)

mlir/tools/mlir-reduce/mlir-reduce.cpp
53

parseSourceFile already verifies the module, so you can remove this.

87

nit: Please use proper punctuation in comments. (In this case, add a period)

msifontes updated this revision to Diff 274943.Wed, Jul 1, 3:46 PM

Add windows specific testing script and test

msifontes updated this revision to Diff 274952.Wed, Jul 1, 4:14 PM
msifontes marked 5 inline comments as done.

Apply requested changes

  • Remove module verification
  • Remove llvm namespace use and explicit const declaration
  • Remove testCase argument duplicate
  • Drop move use
  • Add proper punctuation to comments
Harbormaster completed remote builds in B62597: Diff 274952.

The implementation mlir/tools/mlir-reduce/Tester.cpp does not seems in the right place: it should be somewhere under lib/

msifontes marked 2 inline comments as done.Wed, Jul 1, 5:01 PM

The implementation mlir/tools/mlir-reduce/Tester.cpp does not seems in the right place: it should be somewhere under lib/

Understood, working on it

mehdi_amini added inline comments.Wed, Jul 1, 5:09 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

I don't understand the API contract with the tester right now: why do we provide the original source file here?

msifontes updated this revision to Diff 274984.Wed, Jul 1, 6:27 PM
msifontes marked an inline comment as done.

Move Tester.cpp to lib/Reducer

msifontes updated this revision to Diff 275007.Wed, Jul 1, 10:24 PM
msifontes marked an inline comment as done.

Remove STATIC library declaration

msifontes added inline comments.Wed, Jul 1, 10:51 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

The Tester's run method runs the interestingess script on a given specified file containing a mlir test case variant. This method will be continually invoked every time a transformation is performed on a temporary file containing the new reduced variant. We provide the input file without modification in this call to make sure that it exhibits the interesting behavior. If the initial file does not exhibit the behavior, then no reductions can be performed since any variant won't exhibit the behavior either.

mehdi_amini added inline comments.Wed, Jul 1, 11:22 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

I still don’t understand this: the file was already loaded and you have a module in memory in the tester, why isn’t the tester simply processing the module instead?
Going through file seems inefficient and unnecessary here.

msifontes marked an inline comment as done.Thu, Jul 2, 12:09 AM
msifontes added inline comments.
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

The module is imported into memory to perform the transformations and keep track of the most reduced version. The tester object is the argument to be passed to each reduction pass. The current design uses a user defined testing script which runs with a mlir test case file as an argument to test for the interesting behavior. It would make more sense to run this initial check before setting up the Tester so I can go and make that change. Please let me know if this addresses your concerns or if you know how we could run the interestingness scripts directly on the modules.

mehdi_amini added inline comments.Thu, Jul 2, 9:15 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

The current design uses a user defined testing script which runs with a mlir test case file as an argument to test for the interesting behavior.

Ok, that makes sense that you need to use a file to call the user-provided script, but this does not require to pass a file to the run method as you already have the current most reduced module available, and this is what the test is supposed to use anyway I believe.

msifontes marked an inline comment as done.Thu, Jul 2, 10:56 PM
msifontes added inline comments.
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

The main idea behind having the file as the argument is that the testing script needs a mlir file to interact with as it doesn't have access to the in memory module. I have worked on a utility that could replace this run method: it receives a module as an argument, it then prints the module to a temporary file and runs the testing script using that temporary file as argument. It is important to point out that a module/file is still needed as an argument to the run method to specify which module is being tested. The test is not always run on the most reduced test case. For example, multiple variants can be created at any given point in a pass and those need to be tested and sorted before deciding which is the shortest successful one that will replace the most reduced module.

mehdi_amini added inline comments.Fri, Jul 3, 9:03 AM
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

OK, that makes sense, but I think you need *a lot* more description on the Tester class to capture its intended usage, lifetime, etc.

msifontes updated this revision to Diff 275453.Fri, Jul 3, 12:58 PM
msifontes marked an inline comment as done.

Add Tester class documentation

msifontes marked an inline comment as done.Fri, Jul 3, 1:01 PM
msifontes added inline comments.
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

I have added documentation that I believe clears up the ambiguity in the use of the Tester class. Let me know if there is something that is still not clear form the description.

mehdi_amini added inline comments.Fri, Jul 3, 9:37 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
90

Looks good for now, thanks for your patience in the back and forth!

Nice, looking good

mlir/lib/Reducer/Tester.cpp
36

I dislike using LogicalResult for cases which are just queries. E.g., here failure doesn't mean something exceptional or that this function didn't work/failed. If we aren't returning for failures here (e.g., return failure if result < 0), then it seems like this is a query function:

bool isInteresting(StringRef testCase)

then you also get something like

while (tester.isInteresting(test)) tester.reduce(test)

and then don't have to explain what failure means here as the logic is captured in the name.

50

I think we can set this to not generate the debug dump here (as this is due to a bug in the interestingness test so the stack trace reported from here wouldn't be as useful).

mlir/test/mlir-reduce/testcase.mlir
4 ↗(On Diff #275453)

Can you have both in the same file? I would have expected the two requires to both apply and so nothing runs.

msifontes updated this revision to Diff 275531.Sat, Jul 4, 5:03 PM
msifontes marked 3 inline comments as done.

Apply requested changes

  • Change name of run method to isInteresting
  • Change return type of run method to bool
  • Remove debug dump generation for interestingness error
  • Split test case files for OS specific tests
msifontes updated this revision to Diff 275738.Mon, Jul 6, 9:33 AM
This comment was removed by msifontes.
jpienaar accepted this revision.Mon, Jul 6, 11:04 AM

Looks good thanks

mlir/include/mlir/Reducer/Tester.h
12

Nit: it feels as if parts of this is very specific to class methods below and more useful at that point (e.g., here I don't know it has a run method and perhaps I shouldn't care yet vs knowing it has a way to test interestingness).

Also run method is no more.

msifontes updated this revision to Diff 275770.Mon, Jul 6, 11:11 AM

Fix Tester.cpp dependency issue

msifontes updated this revision to Diff 275777.Mon, Jul 6, 11:44 AM
msifontes marked an inline comment as done.

Modify Tester class documentation

msifontes edited the summary of this revision. (Show Details)Mon, Jul 6, 3:09 PM
msifontes edited the summary of this revision. (Show Details)
rriddle added inline comments.Mon, Jul 6, 7:14 PM
mlir/lib/Reducer/Tester.cpp
36

This is marked as done, but I don't this pattern used anywhere? Are you missing an upload?

msifontes marked 2 inline comments as done.Tue, Jul 7, 8:36 AM
msifontes added inline comments.
mlir/lib/Reducer/Tester.cpp
36

I call this method in line 89 of mlir-reduce.cpp, is this what you are referring to?

msifontes updated this revision to Diff 276097.Tue, Jul 7, 9:14 AM

Remove the windows test case

In order to isolate the build failure cause due to the permission denied when running the testing script.

msifontes updated this revision to Diff 276105.Tue, Jul 7, 9:39 AM

Remove windows specific test case

msifontes updated this revision to Diff 276117.Tue, Jul 7, 10:10 AM

Add windows specific testcase

msifontes updated this revision to Diff 276174.Tue, Jul 7, 12:15 PM

Remove unsupported windows tests

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jul 7, 4:43 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Tue, Jul 7, 5:06 PM
mlir/test/mlir-reduce/testcase-linux.mlir
14

Please terminate files with a new line.

Also what is linux specific to all this?

msifontes marked 2 inline comments as done.Wed, Jul 8, 7:37 AM
msifontes added inline comments.
mlir/test/mlir-reduce/testcase-linux.mlir
14

The execution of the shell script was only working on linux. I tried using a .bat script for windows but the build test was having trouble running the script due to permissions. I will continue to look into how we can test on windows.