Page MenuHomePhabricator

[MLIR][Affine] Add parametric tile size support for affine.for tiling
ClosedPublic

Authored by navdeepkk on Wed, Sep 9, 1:58 AM.

Details

Summary

[MLIR][Affine] Add parametric tile size support for affine.for tiling

Add support to tile affine.for ops with parametric sizes (i.e., SSA
values). Currently supports hyper-rectangular loop nests with constant
lower bounds only. Move methods

  • moveLoopBody(*)
  • getTileableBands(*)
  • checkTilingLegality(*)
  • tilePerfectlyNested(*)
  • constructTiledIndexSetHyperRect(*)

to allow reuse with constant tile size API. Add a test pass -test-affine
-parametric-tile to test parametric tiling.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 9, 1:58 AM
navdeepkk requested review of this revision.Wed, Sep 9, 1:58 AM
navdeepkk edited reviewers, added: bondhugula; removed: nicolasvasilache.Wed, Sep 9, 2:01 AM
bondhugula requested changes to this revision.Wed, Sep 9, 4:11 AM

Thanks for contributing this! Please state at the end of the commit summary as to which methods were just moved as a result of this revision (since there appear to be methods that were purely moved to allow reuse with constant tile size API).

mlir/include/mlir/Transforms/LoopUtils.h
91

Nit: Drop "This function ..". Start with "Checks whether ..>". Please also mention at the commit summary which methods were just moved as a result of this revision.

mlir/test/lib/Transforms/TestAffineLoopParametricTiling.cpp
10

pass -> test pass

or

a pass to test parametric tiling of ..

25

Leave a space below.

66–69

Combine please.

299

Avoid auto.

321

as parameters -> as tile sizes

This revision now requires changes to proceed.Wed, Sep 9, 4:11 AM
bondhugula added inline comments.Wed, Sep 9, 4:14 AM
mlir/test/Dialect/Affine/loop-tiling-parametric.mlir
258

You can do

// CHECK-LABEL: func @tile_with_...
// CHECK-SAME: ([[ARG0:..
bondhugula added inline comments.Wed, Sep 9, 4:17 AM
mlir/include/mlir/Transforms/LoopUtils.h
102–104

Why has this method been exposed instead of being local/static to LoopUtils.cpp? (I don't see it being used anywhere other that.) Similarly, for other methods. They don't have to be exposed in LoopUtils.h unless they are used from other files.

navdeepkk updated this revision to Diff 290734.Wed, Sep 9, 7:01 AM

Address comments on initial diff (290671)

navdeepkk edited the summary of this revision. (Show Details)Wed, Sep 9, 7:05 AM
bondhugula requested changes to this revision.Wed, Sep 9, 7:45 PM
bondhugula added inline comments.
mlir/include/mlir/Transforms/LoopUtils.h
103–112

Please add argument names.

105–108

An extra argument to this method has been added without any documentation. It's not even clear here what it is. Parameter names have also been deleted from the existing one.

This revision now requires changes to proceed.Wed, Sep 9, 7:45 PM
navdeepkk updated this revision to Diff 290923.Thu, Sep 10, 3:21 AM
navdeepkk marked 10 inline comments as done.

Address comments on diff (290734).

Add names to input arguments of functions tilePerfectlyNested and
tilePerfectlyNestedParametric. Change type of input arguments
to these functions to match the coding style.

bondhugula added inline comments.Thu, Sep 10, 10:51 PM
mlir/include/mlir/Transforms/LoopUtils.h
98–99

This is again changing the API for the constant tile size case and this isn't mentioned in the commit summary. Is this intended for this revision? Also, please have the output parameters at the end for consistency. tiledNest has been moved to the front. Ideally, please leave the constant tile size API unchanged in this revision.

103–112

Likewise - fix order of arguments. You can't expose the construction fn input in the API.

104

Why is a user expected to provide a function argument here? This doesn't seem to be meaningful.

navdeepkk updated this revision to Diff 291197.Fri, Sep 11, 6:26 AM
navdeepkk marked 3 inline comments as done.

Address comments on diff (290923).

Move tilePerfectlyNested/tilePerfectlyNestedParametric() back to the
respective passes. Expose moveLoopBody() and checkTilingLegality() in
LoopUtils.h to eliminate redundancy, were removed diff(290734).

navdeepkk updated this revision to Diff 291253.EditedFri, Sep 11, 9:41 AM

Remove auto from appropriate places in TestAffineLoopParametricTiling.cpp

navdeepkk edited the summary of this revision. (Show Details)Fri, Sep 11, 8:56 PM
bondhugula added inline comments.Mon, Sep 14, 9:54 PM
mlir/include/mlir/Transforms/LoopUtils.h
105

I see this comment marked as done but this code is still there.

bondhugula requested changes to this revision.Mon, Sep 14, 9:58 PM
bondhugula added inline comments.
mlir/include/mlir/Transforms/LoopUtils.h
105

These should not be exposed.

mlir/lib/Transforms/Utils/LoopUtils.cpp
1012–1013

You can instead use getOps compactly here.

This revision now requires changes to proceed.Mon, Sep 14, 9:58 PM
bondhugula added inline comments.Mon, Sep 14, 10:00 PM
mlir/lib/Transforms/Utils/LoopUtils.cpp
424–425

This shouldn't be actually returning LogicalResult - but just a bool. Also this should be marked static.

500

This should be marked static as well.

bondhugula added inline comments.Tue, Sep 15, 4:20 AM
mlir/lib/Transforms/Utils/LoopUtils.cpp
506

dest -> dest

mlir/test/lib/Transforms/TestAffineLoopParametricTiling.cpp
276

This is the utility you are adding and this should be in LoopUtils.cpp! and so should all the other local functions this depends on. TestAffineLoopParametricTiling.cpp is just expected to have the test pass logic.

navdeepkk marked 7 inline comments as done.

Address comments on diff (291253).

Split tilePerfecltyNested(*) into performPreTilingChecks(*), constructTiledLoopNest(*),
checkIfHyperRectangular(*) and move all of them to utils to promote code reuse.

navdeepkk edited the summary of this revision. (Show Details)Tue, Sep 15, 10:02 AM
bondhugula requested changes to this revision.Tue, Sep 15, 9:15 PM

constructTiledSetHyper... appears to have been moved but not mentioned in the commit summary as an NFC move. Has it been updated in some way?

This revision now requires changes to proceed.Tue, Sep 15, 9:15 PM
bondhugula added inline comments.Tue, Sep 15, 9:21 PM
mlir/include/mlir/Transforms/LoopUtils.h
108

You don't need LLVM_NODISCARD are you are using it from the test pass here?

mlir/lib/Transforms/Utils/LoopUtils.cpp
569

Type: wether

570

If this function was simply moved, please mention so. If it was refactored, one might as well using the filling ctor for ops.

... ops(input.begin(), input.end());
843

auto -> AffineMap

869

auto -> AffineDimExpr

mlir/test/Dialect/Affine/loop-tiling-parametric.mlir
4

Please add a summary comment on what this test case file is testing and from where the tile sizes are being read.

constructTiledSetHyper... appears to have been moved but not mentioned in the commit summary as an NFC move. Has it been updated in some way?

Sorry, forgot to mention it. It has not been modified in any way just moved to utils because it is being called from tilePerfectlyNested(), which was in turn moved to promote code reuse.

mlir/include/mlir/Transforms/LoopUtils.h
108

Yes, it is being called from line 87 in the test pass.

navdeepkk edited the summary of this revision. (Show Details)Wed, Sep 16, 8:10 AM
bondhugula added inline comments.Wed, Sep 16, 11:51 PM
mlir/lib/Transforms/Utils/LoopUtils.cpp
604–606

Please combine the two.

688

Nit:
bound -> bounds

as the lower -> as the corresponding lower

751–753

Is this comment accurate? The new bound isn't going to be s0 - 5 ceildiv s1 - there should be a + 5 to it?

759–760

Likewise. Anyway, both these cases have the same handling, so I'm not sure why the distinction in the comments.

856

auto -> AffineMap

942

Comment here and a comment below for constructParametr.. - otherwise, it's not immediately clear what this one and what the other does.

mlir/test/lib/Transforms/TestAffineLoopParametricTiling.cpp
15–21

You don't several of these headers I think. Please prune. (AffineValueMap, LoopAnalysis, AffineAnalysis, Analysis/Utils.h etc. may not be needed).

navdeepkk updated this revision to Diff 292429.Thu, Sep 17, 2:11 AM
navdeepkk marked 13 inline comments as done.

Address comments on diff(291964).

bondhugula accepted this revision.Thu, Sep 17, 7:30 AM

Looks good!

mlir/test/lib/Transforms/TestAffineLoopParametricTiling.cpp
17

You won't need Block.h - instead, something else lower is needed perhaps.

This revision is now accepted and ready to land.Thu, Sep 17, 7:30 AM
navdeepkk updated this revision to Diff 292503.Thu, Sep 17, 7:50 AM

Remove include directive "block.h" from "TestAffineLoopParametricTiling.cpp"

bondhugula accepted this revision.Thu, Sep 17, 11:06 AM