Page MenuHomePhabricator

[mlir] Mode for explicitly controlling the fusion kind
ClosedPublic

Authored by sumesh13 on Sep 20 2021, 12:55 PM.

Details

Summary

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.

Diff Detail

Event Timeline

sumesh13 created this revision.Sep 20 2021, 12:55 PM
sumesh13 requested review of this revision.Sep 20 2021, 12:55 PM
bondhugula requested changes to this revision.Mon, Sep 20, 7:38 PM

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.

This revision now requires changes to proceed.Mon, Sep 20, 7:38 PM
bondhugula added inline comments.Mon, Sep 20, 7:44 PM
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?

sumesh13 updated this revision to Diff 374577.Thu, Sep 23, 9:04 AM

Address code review comments

  • Input option as enum
  • Producer Fusion with max src count
sumesh13 marked 11 inline comments as done.Thu, Sep 23, 9:05 AM
sumesh13 added inline comments.
mlir/lib/Transforms/LoopFusion.cpp
1414

Makes sense. renamed as runGreedyFusion

bondhugula added inline comments.Fri, Sep 24, 2:06 AM
mlir/include/mlir/Transforms/Passes.td
142

Not immediately clear how the "greedy" aspect is different from the other two. Rephrase to:
Perform greedy (both producer-consumer and sibling) fusion.?

144–147

Nit: leave a space after commas.

mlir/test/Transforms/loop-fusion-4.mlir
1–2

You don't need to repeat affine-fusion again for the option: just -affine-loop-fusion="mode=producer" should be good enough I think.

7–8

Nit: Use a hyphen: SIBLING-MAXIMAL-...

sumesh13 updated this revision to Diff 374902.Fri, Sep 24, 10:31 AM
sumesh13 marked an inline comment as done.

Address code review comments

sumesh13 marked 4 inline comments as done.Fri, Sep 24, 10:32 AM
bondhugula accepted this revision.Fri, Sep 24, 5:55 PM

LGTM - thanks.

mlir/test/Transforms/loop-fusion-4.mlir
37–42

Nit: hyphen between PRODUCER and CONSUMER.

This revision is now accepted and ready to land.Fri, Sep 24, 5:55 PM
dcaballe accepted this revision.Sun, Sep 26, 2:27 PM

Thanks!

This revision was landed with ongoing or failed builds.Mon, Sep 27, 10:48 AM
This revision was automatically updated to reflect the committed changes.