-
Notifications
You must be signed in to change notification settings - Fork 4
Add automatic tiling #153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add automatic tiling #153
Conversation
EmilySillars
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This PR adds automatic tiling for root operations of IREE dispatches that are
linalg.matmul_transpose_b operations. - Instead of hardcoding the dispatch names and their tiling schemes in the
ConfigureForSnitchpass, we create a modular tiling configuration pass,ConfigureTiles, which queries a table for a tiling scheme each time it encounters a dispatch with a root operation oflinlag.matmul_transpose_b. The table gets populated based on a JSON file provided by the user as a command line argument, before ConfigureTiles runs.
Relevant Files:
ConfigureTiles.cppandPasses.td(defining modular configuration pass)TilingScheme.cppandTilingScheme.h(defines a struct representing a tiling scheme, and how to populate the table of structs given a json file input)
| // import any manually supplied tile sizes | ||
| if (targetOptions.importTiles != "") { | ||
| std::string errs; | ||
| quidditch::fillTileInfoTable(&targetOptions.tileInfo, | ||
| targetOptions.importTiles, errs); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import tiling schemes and populate the table before creating the ConfigureTiles pass
| Option<"tablePointer", "NeverPassAValueHere", "std::uintptr_t", /*default=*/"0", | ||
| "Avoids opening the input file multiple times. Never pass a value to this option via the command line.">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the tiling schemes are passed in by the user in a single json file, it made sense to me that before constructing the ConfigureTiles pass, that json file should be read once and its information populated in a lookup table. This way, each time the ConfigureTiles pass runs over a functionOp, it doesn't have to open and read the contents of the same file again.
I couldn't figure out an easy way to provide another argument to the ConfigureTiles pass without also exposing it to the user. Is it okay to leave it as is, or should I work harder and find a way to remove this as a command line argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that I see the advantage, why not read it in the pass itself?
| quidditch_module(SRC ${CMAKE_CURRENT_BINARY_DIR}/nsnet2.mlirbc DST nsnet2 FLAGS --iree-quidditch-import-tiles=${CMAKE_CURRENT_LIST_DIR}/manually-chosen-tiles.json) | ||
| quidditch_module(SRC ${CMAKE_CURRENT_BINARY_DIR}/nsnet2.mlirbc LLVM DST nsnet2_llvm FLAGS --iree-quidditch-import-tiles=${CMAKE_CURRENT_LIST_DIR}/manually-chosen-tiles.json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where the user provides Quidditch with all the tiling schemes in a single json file.
| @@ -0,0 +1,27 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file defines how to tile all the matmul transpose operations inside NsNet2
|
@superlopuh @compor could one of you review this smaller PR? I don't think I have permissions to add you as reviewers. Summary of changes in this comment here: #153 (review) |
No description provided.