Page MenuHomePhabricator

[MLIR] Parallelize affine.for op to 1-D affine.parallel op
ClosedPublic

Authored by yash on Fri, Jun 26, 11:37 AM.

Details

Summary

Introduce pass to convert parallel affine.for op into 1-D affine.parallel op. Run using --affine-parallelize. Removes test-detect-parallel: pass for checking parallel affine.for ops.

Signed-off-by: Yash Jain <yash.jain@polymagelabs.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bondhugula retitled this revision from Parallelize affine.for op to 1-D affine.parallel op to [MLIR] Parallelize affine.for op to 1-D affine.parallel op.Fri, Jun 26, 11:57 AM
rriddle added inline comments.Fri, Jun 26, 12:16 PM
mlir/lib/Dialect/Affine/Transforms/AffineParallelize.cpp
8

This header looks incorrect, can you copy over from another file?

40

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

42

This doesn't look correct, can you look at some of the other passes as an example?

50

nit: Operation* -> AffineForOp?

52

nit: Please drop trivial braces here and below.

56

Can you sink this into the affineParallelize utility instead?

58

nit: Drop braces here.

mlir/lib/Transforms/Utils/LoopUtils.cpp
1533 ↗(On Diff #273798)

nit: /// for function level comments.

1538 ↗(On Diff #273798)

nit: Please use complete sentences for comments, this applies to all comments in the patch.

1542 ↗(On Diff #273798)

nit: Spell out auto here and below.

1547 ↗(On Diff #273798)

Why not move the body directly and then replace all uses of the arguments? That would remove the need to clone every internal operation, which can get very costly. E.g., if the loop has nested loops inside of it.

1549 ↗(On Diff #273798)

nit: Drop trivial braces.

1549 ↗(On Diff #273798)

nit: Spell out auto here.

1551 ↗(On Diff #273798)

nit: Can you drop all of the trivial braces?

flaub added a subscriber: flaub.Fri, Jun 26, 12:50 PM
yash updated this revision to Diff 275081.Thu, Jul 2, 6:01 AM
yash marked 13 inline comments as done.

Minor Updates
Removed trivial braces
Changed header of the file

bondhugula added inline comments.Thu, Jul 2, 6:55 AM
mlir/lib/Dialect/Affine/Transforms/AffineParallelize.cpp
31

Nit: affine forOp -> affine.for op

46

You don't need the cast any more.

mlir/test/Dialect/Affine/parallelize.mlir
100

Looks like this is not fixed yet.

ftynse requested changes to this revision.Thu, Jul 2, 9:16 AM
ftynse added inline comments.
mlir/include/mlir/Transforms/LoopUtils.h
161 ↗(On Diff #275081)

Any reason why this cannot live under Dialect/Affine/Transforms/AffineParallelize instead? I know there are some affine-specific functions in this file, but these are refactoring leftovers....

mlir/lib/Dialect/Affine/Transforms/AffineParallelize.cpp
39

This looks unused

45

Please don't use auto unless it improves readability, AffineForOp would do just fine here (and also help you notice the unnecessary cast below)

mlir/lib/Transforms/Utils/LoopUtils.cpp
1550 ↗(On Diff #275081)

Since this is not happening in the pattern rewriter anyway (if it had, it would have required you to pass in an OpBuilder instance that must be used), there's no point in cloning. You can just splice operations from one block to another (look at the Block::splice documentation for proper usage) and call forOpIV.replaceAllUsesWith(parallelIV).

If you want this to be callable from the pattern rewriting infra, then make sure it takes an OpBuilder & to use instead of creating a new one, and do all transformations through this build (e.g., no calls to .erase()).

1553 ↗(On Diff #275081)

Did you want to also dump the loop? Otherwise this decoration looks excessive

This revision now requires changes to proceed.Thu, Jul 2, 9:16 AM
bondhugula requested changes to this revision.Thu, Jul 2, 1:50 PM
bondhugula added inline comments.
mlir/lib/Transforms/Utils/LoopUtils.cpp
1550 ↗(On Diff #275081)

Actually, one could just use Block::moveBefore or Region::takeBody? - that way you don't need to splice or even replace all uses. (Since this is just for 1-d, the region arg count will also match.)

yash updated this revision to Diff 275289.Thu, Jul 2, 9:52 PM
yash marked 6 inline comments as done.

Minor Updates:
Moving instead of cloning.
Completed some comments.

yash updated this revision to Diff 275290.Thu, Jul 2, 10:05 PM
yash marked 5 inline comments as done.

Removed unused lines.

Harbormaster completed remote builds in B62801: Diff 275290.
ftynse accepted this revision.Fri, Jul 3, 1:03 AM

Some minor comments.

mlir/lib/Transforms/Utils/LoopUtils.cpp
1538 ↗(On Diff #275290)

Regular '' comment instead of /.

1542 ↗(On Diff #275290)

Nit: This can be a regular '//' comment.

mlir/test/Dialect/Affine/parallelize.mlir
4

Complete sentence please.

115

Nit: no need to capture arg1 - it's unused.

yash updated this revision to Diff 275389.Fri, Jul 3, 7:10 AM
yash marked 3 inline comments as done.

Minor Updates in comments

yash updated this revision to Diff 275486.Sat, Jul 4, 2:29 AM
yash marked 3 inline comments as done.

Marked done.

bondhugula requested changes to this revision.Sat, Jul 4, 3:09 AM
bondhugula added inline comments.
mlir/include/mlir/Transforms/LoopUtils.h
161 ↗(On Diff #275081)

Actually, this can go under an Dialect/Affine/Utils/Utils.cpp?

157–159 ↗(On Diff #275486)

This comment is inaccurate (all affine for ops)? The second sentence is grammatically broken. Also 1-D affine ParallelOp -> 1-d affine.parallel op.

This revision now requires changes to proceed.Sat, Jul 4, 3:09 AM
yash updated this revision to Diff 275492.Sat, Jul 4, 4:47 AM
yash marked an inline comment as done.

Shift code from
include/mlir/Transforms/LoopUtils.h to include/mlir/Dialect/Affine/Utils.h
lib/Transforms/Utils/LoopUtils.cpp to lib/Dialect/Affine/Utils/Utils.cpp

yash updated this revision to Diff 275493.Sat, Jul 4, 4:52 AM
yash marked 2 inline comments as done.

Minor updates.

bondhugula added inline comments.Sat, Jul 4, 5:01 AM
mlir/include/mlir/Dialect/Affine/Utils.h
23

Typo 1-s

24

isLoopParallel does not give but is only used to detect.

25

Please use a complete sentence. "There is no cost model currently used ..."

mlir/lib/Dialect/Affine/Transforms/AffineParallelize.cpp
32

Please consistently use affine.parallel op (insetad of affine parallelOp and other variations).

mlir/lib/Dialect/Affine/Utils/Utils.cpp
132

This comment is out of data and inaccurate now.

137

Nit: Creating -> Create

yash updated this revision to Diff 275494.Sat, Jul 4, 5:17 AM

Comments updated.

yash updated this revision to Diff 275495.Sat, Jul 4, 5:18 AM
yash marked 6 inline comments as done.

Marked done.

yash updated this revision to Diff 275496.Sat, Jul 4, 5:21 AM

In clang-format confirmation.

yash updated this revision to Diff 275498.Sat, Jul 4, 5:24 AM

Removing blank line.

bondhugula accepted this revision.Sat, Jul 4, 5:26 AM
This revision is now accepted and ready to land.Sat, Jul 4, 5:26 AM
Harbormaster completed remote builds in B62897: Diff 275493.
Harbormaster completed remote builds in B62899: Diff 275495.
Harbormaster completed remote builds in B62900: Diff 275496.
bondhugula edited the summary of this revision. (Show Details)Sat, Jul 4, 6:23 AM
This revision was automatically updated to reflect the committed changes.

Reverted in fbc06b228012 ; this broke the build with the -DDBUILD_SHARED_LIBS=ON configuration.

Reverted in fbc06b228012 ; this broke the build with the -DDBUILD_SHARED_LIBS=ON configuration.

If I'm not mistaken, all harbormaster build tests passed above. If shared lib building isn't being tested as part of it, are developers expected to maintain two builds (w/ and w/o shared libs) and test on both before committing?

mehdi_amini added a comment.EditedSun, Jul 5, 11:10 AM

The pre-merge is a sanity check, it does not provide complete coverage of the supported configurations and platforms.
Think about the configuration matrix of:

  • All versions of gcc > 5.1 + all version of clang
  • With and without assertions
  • Various Sanitizers
  • Linking tools against the shared library ( -DLLVM_LINK_LLVM_DYLIB=ON )
  • With/without specific targets configured in
  • libstdc++ vs libc++?

You may also want to test on a Arm64 machine, on a X86-32bits, on an big-endian powerpc, ...

That's only on Linux, now add Windows with multiple MSVC versions, MacOS, FreeBSD ...

The pre-merge is a sanity check, it does not provide complete coverage of the supported configurations and platforms.
Think about the configuration matrix of:

  • All versions of gcc > 5.1 + all version of clang
  • With and without assertions
  • Various Sanitizers
  • Linking tools against the shared library ( -DLLVM_LINK_LLVM_DYLIB=ON )
  • With/without specific targets configured in
  • libstdc++ vs libc++?

    You may also want to test on a Arm64 machine, on a X86-32bits, on an big-endian powerpc, ...

    That's only on Linux, now add Windows with multiple MSVC versions, MacOS, FreeBSD ...

Yes, I'm aware of that. But shouldn't DYLIB be given higher priority given the higher likelihood of a build failure when compared to other combinations?