This is an archive of the discontinued LLVM Phabricator instance.

Differential D82803

Create the MLIR Reduce framework
ClosedPublic

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

Details

Reviewers
jpienaar
tpopp
rriddle
Commits
rGec04ce462352: Create the MLIR Reduce framework
rG28a45d54a7fe: Create the framework and testing environment for MLIR Reduce - a tool with the…
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

msifontes created this revision.Jun 29 2020, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 1:25 PM
Herald added subscribers: tpopp, jurahul, Kayjukh and 15 others. · View Herald Transcript
msifontes updated this revision to Diff 274222.Jun 29 2020, 1:31 PM

#Updating D82803: Create the MLIR Reduce framework

Uncomment the canonicalization pass of the MLIR module.

Harbormaster failed remote builds in B62209: Diff 274221!Jun 29 2020, 2:13 PM
Harbormaster failed remote builds in B62210: Diff 274222!
msifontes added reviewers: jpienaar, tpopp.Jun 29 2020, 2:14 PM
msifontes updated this revision to Diff 274236.Jun 29 2020, 2:20 PM

Updating D82803: Create the MLIR Reduce framework

Change variable names to comply with camelCase formatting.

rriddle requested changes to this revision.Jun 29 2020, 2:31 PM

Thanks Mauricio, added some initial comments.

mlir/tools/mlir-reduce/Tester.cpp
17 ↗(On Diff #274236)

Please avoid using namespace llvm whenever you can.

28 ↗(On Diff #274236)

nit: Spell out auto here.

mlir/tools/mlir-reduce/Tester.h
1 ↗(On Diff #274236)

nit: Remove this newline.

22 ↗(On Diff #274236)

Please trim all of the headers here that you can.

28 ↗(On Diff #274236)

Please do not having using statements like this in a header file.

31 ↗(On Diff #274236)

Please properly wrap this in the mlir namespace.

31 ↗(On Diff #274236)

Can you please add proper documentation for this class?

32 ↗(On Diff #274236)

nit: Remove this newline.

34 ↗(On Diff #274236)

nit: Use ArrayRef here instead of std::vector.

47 ↗(On Diff #274236)

ArrayRef here as well.

48 ↗(On Diff #274236)

Seems like you want an OwningModuleRef here and not a ModuleOp.

mlir/tools/mlir-reduce/mlir-reduce.cpp
24 ↗(On Diff #274236)

nit: Could you please try to trim some of these headers?

41 ↗(On Diff #274236)

Same here, please remove the using llvm.

57 ↗(On Diff #274236)

nit: Please use static if the function is meant to be internal.

60 ↗(On Diff #274236)

nit: Swap these two conditions and return early:

if (!...)
  return;
72 ↗(On Diff #274236)

nit: Remove this newline.

74 ↗(On Diff #274236)

nit: Same here.

78 ↗(On Diff #274236)

nit: You don't seem to be doing anything with this pass manager.

This revision now requires changes to proceed.Jun 29 2020, 2:31 PM
Harbormaster failed remote builds in B62215: Diff 274236!Jun 29 2020, 2:46 PM
mehdi_amini added a comment.Jun 29 2020, 4:07 PM

If the tools is intended to be extended, I suspect that classes like class Tester should be in a library (and so the header be under the include folder).
The tools folder is only for the generated binary, I'd follow what we do with mlir-opt as a pattern here.

msifontes updated this revision to Diff 274284.Jun 29 2020, 4:31 PM
msifontes marked 17 inline comments as done.

Updating D82803: Create the MLIR Reduce framework

Apply changes specified in review

  • Trim headers
  • Replace std:::vector use with llvm::RefArray
  • Apply formatting changes
  • Add Tester class documentation
  • Erase llvm namespace uses
stephenneuendorffer added inline comments.Jun 29 2020, 4:33 PM
mlir/tools/mlir-reduce/CMakeLists.txt
41–45 ↗(On Diff #274236)

DEPENDS is unnecessary. All target_link_libraries are implicitly dependencies.

Harbormaster failed remote builds in B62247: Diff 274284!Jun 29 2020, 4:54 PM
msifontes updated this revision to Diff 274319.Jun 29 2020, 8:13 PM

Updating D82803: Create the MLIR Reduce framework

Delete unnecessary dependency statement in the CMakeLists.txt file

Harbormaster failed remote builds in B62267: Diff 274319!Jun 29 2020, 8:56 PM
tpopp added inline comments.Jun 30 2020, 7:10 AM
mlir/test/CMakeLists.txt
44 ↗(On Diff #274319)

Alphabetical order. This is weird because of the dashes and underscores, but in this case, it goes 3 lines up.

mlir/test/mlir-reduce/testcase.mlir
1 ↗(On Diff #274319)

%p/testcase.mlir can just be %s

mlir/tools/CMakeLists.txt
9 ↗(On Diff #274319)

nit: alphabetic order here also, although the previous line isn't sorted either.

mlir/tools/mlir-reduce/CMakeLists.txt
1 ↗(On Diff #274319)

Is this supposed to be here?

24 ↗(On Diff #274319)

Alphabetize. Maybe it's not expected with LLVM, but it's a good habit to get into.

mlir/tools/mlir-reduce/mlir-reduce.cpp
48 ↗(On Diff #274319)

If possible, using this enum and helpers would be nice. http://google3/third_party/llvm/llvm-project/mlir/include/mlir/Support/LogicalResult.h?l=19

53 ↗(On Diff #274319)

parseSourceFile might return a nullptr.

msifontes updated this revision to Diff 274486.Jun 30 2020, 8:11 AM
msifontes marked 7 inline comments as done.

Applied Review changes

  • Alphabetize dependencies
  • Replace filename with %s in testcase.mlir
  • Use LogicalResult enum for error reporting
  • Add check for nullptr parsing result
mehdi_amini added a comment.Jun 30 2020, 8:45 AM
In D82803#2121353, @mehdi_amini wrote:

If the tools is intended to be extended, I suspect that classes like class Tester should be in a library (and so the header be under the include folder).
The tools folder is only for the generated binary, I'd follow what we do with mlir-opt as a pattern here.

Ping here, in case you missed it?

msifontes marked an inline comment as done.Jun 30 2020, 8:55 AM
In D82803#2123056, @mehdi_amini wrote:
In D82803#2121353, @mehdi_amini wrote:

If the tools is intended to be extended, I suspect that classes like class Tester should be in a library (and so the header be under the include folder).
The tools folder is only for the generated binary, I'd follow what we do with mlir-opt as a pattern here.

Ping here, in case you missed it?

Thanks Mehdi, I am currently looking at mlir-opt and making the change

msifontes updated this revision to Diff 274511.EditedJun 30 2020, 9:07 AM

Updating D82803: Create the MLIR Reduce framework

Move Tester.h to include/Support directory

Harbormaster failed remote builds in B62333: Diff 274486!Jun 30 2020, 9:12 AM
jpienaar added a comment.Jun 30 2020, 9:20 AM
In D82803#2123102, @msifontes wrote:
In D82803#2123056, @mehdi_amini wrote:
In D82803#2121353, @mehdi_amini wrote:

If the tools is intended to be extended, I suspect that classes like class Tester should be in a library (and so the header be under the include folder).
The tools folder is only for the generated binary, I'd follow what we do with mlir-opt as a pattern here.

Ping here, in case you missed it?

Thanks Mehdi, I am currently looking at mlir-opt and making the change

I think the proposal is to have a include/mlir/Reducer to mirror TableGen side - at least for me include/mlir/Support is a bit broad. But wait for others comments before moving, I'm not sure if this is what Mehdi ment.

jpienaar added a comment.Jun 30 2020, 9:43 AM

Looks like a good start :)

mlir/include/mlir/Support/Tester.h
15 ↗(On Diff #274511)

You'll need to update this post the move.

37 ↗(On Diff #274511)

Could we use something other than int and also document what the returns status signifies?

43 ↗(On Diff #274511)

setTestCase to me would just mean to update whatever is being tested, if this is meant to only be called to update the most reduced, then the function name doesn't convey that to me.

mlir/tools/mlir-reduce/Tester.cpp
44 ↗(On Diff #274511)

It seems like either you can return (although I don't know what int return signifies here :) ) or use llvm::report_fatal_error here.

mlir/tools/mlir-reduce/mlir-reduce.cpp
35 ↗(On Diff #274511)

<input file> to match mlir-opt ?

39 ↗(On Diff #274511)

Interestingness ? (or just Testing script as I don't think it is overloaded here and there will only be one).

43 ↗(On Diff #274511)

Same as above

46 ↗(On Diff #274511)

Output filename for reduced test case ?

50 ↗(On Diff #274511)

Nit: How about loadModule?

52 ↗(On Diff #274511)

Why do we care? E.g., if it is in a .txt file and still parses it seems fine.

63 ↗(On Diff #274511)

I'd just for the load in the main function and then have the processing/reduction in a next. I don't think much is gained in terms of reuse or readabability from loadAndProcessMLIR vs a loadModule and reduceReproducer calls in main.

69 ↗(On Diff #274511)

Should we just remove these for now? Seems like nops as passmanager isn't run).

91 ↗(On Diff #274511)

Just use report_fatal_errors here for simplicity as you are in the main binary file (removes need for trailing newline too, yes we don't do that consistently in mlir-opt ...)

Harbormaster failed remote builds in B62350: Diff 274511!Jun 30 2020, 10:18 AM
mehdi_amini added a comment.Jun 30 2020, 10:47 AM

I think the proposal is to have a include/mlir/Reducer to mirror TableGen side - at least for me include/mlir/Support is a bit broad. But wait for others comments before moving, I'm not sure if this is what Mehdi ment.

Spot on, thanks!

msifontes updated this revision to Diff 274560.Jun 30 2020, 11:45 AM
msifontes marked 13 inline comments as done.

Updating D82803: Create the MLIR Reduce framework

Apply requested changes

  • Use llvm::report_fatal_error for error reporting
  • Use LogicalResult value to indicate the result of the interestingness script
  • Change testCase name to mostReduced to improve readability
  • Remove loadAndProcessMLIR method in favor of moving the canonicalization pass to a reducer call
Harbormaster failed remote builds in B62377: Diff 274560!Jun 30 2020, 12:29 PM
msifontes updated this revision to Diff 274588.Jun 30 2020, 12:37 PM

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.EditedJun 30 2020, 1:51 PM

Modify testing script so that it is not OS specific

Harbormaster failed remote builds in B62387: Diff 274588!Jun 30 2020, 2:09 PM
Harbormaster failed remote builds in B62401: Diff 274608!Jun 30 2020, 3:14 PM
msifontes updated this revision to Diff 274660.Jun 30 2020, 5:35 PM

Normalize path separators

Harbormaster failed remote builds in B62437: Diff 274660!Jun 30 2020, 6:28 PM
msifontes updated this revision to Diff 274879.Jul 1 2020, 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.

Harbormaster failed remote builds in B62553: Diff 274879!Jul 1 2020, 11:54 AM
rriddle added inline comments.Jul 1 2020, 1:18 PM
mlir/include/mlir/Reducer/Tester.h
28 ↗(On Diff #274879)

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

34 ↗(On Diff #274879)

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.

43 ↗(On Diff #274879)

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
52 ↗(On Diff #274879)

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

86 ↗(On Diff #274879)

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

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

Add windows specific testing script and test

msifontes updated this revision to Diff 274952.Jul 1 2020, 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 B62591: Diff 274943.Jul 1 2020, 4:45 PM
Harbormaster completed remote builds in B62597: Diff 274952.
mehdi_amini added a comment.Jul 1 2020, 4:56 PM

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.Jul 1 2020, 5:01 PM
In D82803#2126922, @mehdi_amini wrote:

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.Jul 1 2020, 5:09 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.Jul 1 2020, 6:27 PM
msifontes marked an inline comment as done.

Move Tester.cpp to lib/Reducer

Harbormaster failed remote builds in B62616: Diff 274984!Jul 1 2020, 7:26 PM
msifontes updated this revision to Diff 275007.Jul 1 2020, 10:24 PM
msifontes marked an inline comment as done.

Remove STATIC library declaration

msifontes added inline comments.Jul 1 2020, 10:51 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.

Harbormaster failed remote builds in B62631: Diff 275007!Jul 1 2020, 11:15 PM
mehdi_amini added inline comments.Jul 1 2020, 11:22 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.Jul 2 2020, 12:09 AM
msifontes added inline comments.
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.Jul 2 2020, 9:15 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.Jul 2 2020, 10:56 PM
msifontes added inline comments.
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.Jul 3 2020, 9:03 AM
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.Jul 3 2020, 12:58 PM
msifontes marked an inline comment as done.

Add Tester class documentation

msifontes marked an inline comment as done.Jul 3 2020, 1:01 PM
msifontes added inline comments.
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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.

Harbormaster failed remote builds in B62879: Diff 275453!Jul 3 2020, 1:30 PM
mehdi_amini added inline comments.Jul 3 2020, 9:37 PM
mlir/tools/mlir-reduce/mlir-reduce.cpp
89 ↗(On Diff #274952)

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

jpienaar added a comment.Jul 4 2020, 9:57 AM

Nice, looking good

mlir/lib/Reducer/Tester.cpp
35 ↗(On Diff #275453)

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.

49 ↗(On Diff #275453)

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.Jul 4 2020, 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
Harbormaster failed remote builds in B62923: Diff 275531!Jul 4 2020, 5:51 PM
msifontes updated this revision to Diff 275738.Jul 6 2020, 9:33 AM
This comment was removed by msifontes.
Harbormaster failed remote builds in B63036: Diff 275738!Jul 6 2020, 9:44 AM
jpienaar accepted this revision.Jul 6 2020, 11:04 AM

Looks good thanks

mlir/include/mlir/Reducer/Tester.h
11 ↗(On Diff #275531)

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.Jul 6 2020, 11:11 AM

Fix Tester.cpp dependency issue

Harbormaster failed remote builds in B63051: Diff 275770!Jul 6 2020, 11:29 AM
msifontes updated this revision to Diff 275777.Jul 6 2020, 11:44 AM
msifontes marked an inline comment as done.

Modify Tester class documentation

msifontes edited the summary of this revision. (Show Details)Jul 6 2020, 3:09 PM
msifontes edited the summary of this revision. (Show Details)
rriddle added inline comments.Jul 6 2020, 7:14 PM
mlir/lib/Reducer/Tester.cpp
35 ↗(On Diff #275453)

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

msifontes edited commits, added rG28a45d54a7fe: Create the framework and testing environment for MLIR Reduce - a tool with the…; removed rGa5a8c1ed5660: Unreachable (removed).Jul 7 2020, 8:31 AM
msifontes marked 2 inline comments as done.Jul 7 2020, 8:36 AM
msifontes added inline comments.
mlir/lib/Reducer/Tester.cpp
35 ↗(On Diff #275453)

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.Jul 7 2020, 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.

Harbormaster failed remote builds in B63230: Diff 276097!Jul 7 2020, 9:15 AM
msifontes updated this revision to Diff 276105.Jul 7 2020, 9:39 AM

Remove windows specific test case

Harbormaster failed remote builds in B63234: Diff 276105!Jul 7 2020, 9:40 AM
msifontes updated this revision to Diff 276117.Jul 7 2020, 10:10 AM

Add windows specific testcase

Harbormaster failed remote builds in B63241: Diff 276117!Jul 7 2020, 10:10 AM
mehdi_amini added a comment.Jul 7 2020, 10:25 AM

reverted in e10e034f4bb7d3c639cc65a8d60147ff1e330e56

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

Remove unsupported windows tests

Harbormaster completed remote builds in B63275: Diff 276174.Jul 7 2020, 3:50 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 7 2020, 4:43 PM
Closed by commit rGec04ce462352: Create the MLIR Reduce framework (authored by msifontes). · Explain Why
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jul 7 2020, 5:06 PM
mlir/test/mlir-reduce/testcase-linux.mlir
14 ↗(On Diff #276263)

Please terminate files with a new line.

Also what is linux specific to all this?

msifontes marked 2 inline comments as done.Jul 8 2020, 7:37 AM
msifontes added inline comments.
mlir/test/mlir-reduce/testcase-linux.mlir
14 ↗(On Diff #276263)

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.

Revision Contents

PathSize
mlir/
test/
mlir-reduce/

Diff 276097

mlir/test/mlir-reduce/testcase-windows.mlir

  • This file was deleted.
// UNSUPPORTED: -linux-
// RUN: mlir-reduce %s -test %S/test.bat
func @simple1(%arg0: i1, %arg1: memref<2xf32>, %arg2: memref<2xf32>) {
cond_br %arg0, ^bb1, ^bb2
^bb1:
br ^bb3(%arg1 : memref<2xf32>)
^bb2:
%0 = alloc() : memref<2xf32>
br ^bb3(%0 : memref<2xf32>)
^bb3(%1: memref<2xf32>):
return
}
No newline at end of file