New mode option that allows for either running the default fusion kind that happens today or doing either of producer-consumer or sibling fusion. This will also be helpful to minimize the compile-time of the fusion tests.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks for adding this. Please use enum valued cl option to support this.
mlir/include/mlir/Transforms/Passes.h | ||
---|---|---|
82 | Missing documentation for new arg. | |
mlir/include/mlir/Transforms/Passes.td | ||
139 | Nit: space after comma. | |
139 | Please avoid using hard coded values like 0, 1, ... as pass options. It's not readable when used in run commands, textual pass pipelines, etc. Please instead use enum valued command-line options. See test-legalize-mode for reference.. | |
140 | Producer- -> producer-> | |
mlir/lib/Transforms/LoopFusion.cpp | ||
1409–1414 | All functions are supposed to have doc comments. |
mlir/include/mlir/Transforms/Passes.h | ||
---|---|---|
85–86 | Why is this a bool? Don't you have three modes? If this is a bug, the testing isn't adequate. | |
mlir/lib/Transforms/LoopFusion.cpp | ||
67 | How has unsigned become a bool here?! | |
97–98 | Triple /// comments. | |
mlir/test/Transforms/loop-fusion-4.mlir | ||
1–3 | Use enum valued command line options. Eg: mlir-opt -h | grep test-legalize-mode -A 3 --test-legalize-mode=<value> - The legalization mode to use with the test driver =analysis - Perform an analysis conversion =full - Perform a full conversion =partial - Perform a partial conversion |
Thanks, Sumesh! A couple of comments...
mlir/lib/Transforms/LoopFusion.cpp | ||
---|---|---|
1414 | Probably good to rename this now to something more meaningful. | |
1415 | IIRC, maxSrcUserCount = 1 was used with the purpose of enabling some sibling fusion scenarios. I think this should be std::numeric_limits<unsigned>::max() instead? |
mlir/lib/Transforms/LoopFusion.cpp | ||
---|---|---|
1414 | Makes sense. renamed as runGreedyFusion |
mlir/include/mlir/Transforms/Passes.td | ||
---|---|---|
142 | Not immediately clear how the "greedy" aspect is different from the other two. Rephrase to: | |
144–147 | Nit: leave a space after commas. | |
mlir/test/Transforms/loop-fusion-4.mlir | ||
1 | You don't need to repeat affine-fusion again for the option: just -affine-loop-fusion="mode=producer" should be good enough I think. | |
41 | Nit: Use a hyphen: SIBLING-MAXIMAL-... |
LGTM - thanks.
mlir/test/Transforms/loop-fusion-4.mlir | ||
---|---|---|
78–83 | Nit: hyphen between PRODUCER and CONSUMER. |
Missing documentation for new arg.