This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Adds affine loop fusion transformation function to LoopFusionUtils.
ClosedPublic

Authored by andydavis1 on Jan 22 2020, 6:43 AM.

Details

Summary

Adds affine loop fusion transformation function to LoopFusionUtils.
Updates TestLoopFusion utility to run loop fusion transformation until a fixed point is reached.
Adds unit tests to test the transformation.

Diff Detail

Event Timeline

andydavis1 created this revision.Jan 22 2020, 6:43 AM

Unit tests: pass. 62057 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

rriddle added inline comments.Jan 22 2020, 8:47 AM
mlir/lib/Transforms/Utils/LoopFusionUtils.cpp
258

nit: Add newlines before comments.

272

Remove trivial braces.

mlir/test/lib/Transforms/TestLoopFusion.cpp
193

Remove trivial braces.

dcaballe requested changes to this revision.Jan 22 2020, 9:21 AM

Thanks for working on this Andy! Some minor comments

mlir/test/Transforms/loop-fusion-transformation.mlir
3

nit: remove this first ----- so that we don't process an empty chunk?

17

nit: for all these tests, please remove unnecessary lhs (e.g., %{{.*}} =) for loads, addf, and other ops if we don't capture their output in a variable

mlir/test/lib/Transforms/TestLoopFusion.cpp
187

Would it make sense to have only the fixed point optimization "mode"? Do we need to keep the old mode for some reason (other than, maybe, fixing some tests)?
If so, probably better to create an independent pass (TestFixedPointLoopFusion?) for the fixed point mode since it's independent from the other mode. In that way, we wouldn't need to use both flags -test-loop-fusion -test-loop-fusion-transformation for testing.

196

Trye -> Try

This revision now requires changes to proceed.Jan 22 2020, 9:21 AM

Adds affine loop fusion transformation function to LoopFusionUtils.

Can you clarify here if it's moving something as opposed to introducing something new?

bondhugula added inline comments.Jan 23 2020, 1:58 AM
mlir/include/mlir/Transforms/LoopFusionUtils.h
56

A bit strange to have the destination insertion point stored in srcSlice! Separate arg?

mlir/lib/Transforms/Utils/LoopFusionUtils.cpp
257

srcForOp should do?

mlir/test/lib/Transforms/TestLoopFusion.cpp
45

"test-loop-fusion" sounds fine?

andydavis1 marked 10 inline comments as done.Jan 30 2020, 9:46 AM

thanks. Sending update ...

mlir/include/mlir/Transforms/LoopFusionUtils.h
56

They are inter-dependent: each source slice is computed for a specific dest insertion point.

mlir/test/lib/Transforms/TestLoopFusion.cpp
45

That name collides with another flag name. Eventually, we'll be able to get rid of the other test modes and collapse things down to one flag, but probably too soon right now.

187

Once things stabilize, we can move towards having a fixed point mode. For now, its nice to have specific tests that check slice computation and fusion dependence checks.

addresing comments

Unit tests: pass. 62323 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dcaballe accepted this revision.Feb 3 2020, 2:52 PM

LGTM. Just a minor suggestion.

mlir/test/lib/Transforms/TestLoopFusion.cpp
187

Ok, it makes sense to me, thanks! I would still suggest moving the transformation part (i.e, the code under the if condition (lines 187-200) to its own pass within this file so that we don't have to use both flags to test the transformation flavor. That code seems independent from the rest of the test pass.

This revision is now accepted and ready to land.Feb 3 2020, 2:52 PM
bondhugula accepted this revision.Feb 3 2020, 3:12 PM

LGTM - thanks!

andydavis1 updated this revision to Diff 242777.Feb 5 2020, 3:52 PM

rebasing with head

This revision was automatically updated to reflect the committed changes.

Unit tests: unknown.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Reverted in 2724ada8d2ddfec8f1e3d39f1d02f8b5d5224f1b as this broke the ASAN checker for a use-after-free in mlir::canFuseLoops(mlir::AffineForOp, mlir::AffineForOp, unsigned int, mlir::ComputationSliceState*) lib/Transforms/Utils/LoopFusionUtils.cpp:202:41

Thanks. A fix is on the way...