This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a simplifying wrapper for generateCopy and expose it.
ClosedPublic

Authored by timshen on Mar 10 2020, 4:03 PM.

Details

Summary

affineDataCopyGenerate is a monolithinc function that
combines several steps for good reasons, but it makes customizing
the behaivor even harder. The major two steps by affineDataCopyGenerate are:
a) Identify interesting memrefs and collect their uses.
b) Create new buffers to forward these uses.

Step (a) actually has requires tremendous customization options. One could see
that from the recently added filterMemRef parameter.

This patch adds a function that only does (b), in the hope that (a)
can be directly implemented by the callers. In fact, (a) is quite
simple if the caller has only one buffer to consider, or even one use.

Diff Detail

Event Timeline

timshen created this revision.Mar 10 2020, 4:03 PM
timshen updated this revision to Diff 249514.Mar 10 2020, 4:17 PM

Update comments.

timshen planned changes to this revision.Mar 10 2020, 4:36 PM

I'm going to add some tests.

rriddle added inline comments.Mar 10 2020, 4:37 PM
mlir/include/mlir/Transforms/LoopUtils.h
18

Can you can forward declare the necessary things and move this header to the .cpp file?

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

Please drop trivial braces.

1820

Why not rework this?

assert(copyNests.size() <= 1 && ...);
result.copy_nest = copyNests.empty() ? nullptr : *copyNests.begin();

timshen updated this revision to Diff 249526.Mar 10 2020, 5:37 PM

Add a test.

timshen updated this revision to Diff 249528.Mar 10 2020, 5:43 PM
timshen marked an inline comment as done.

Address comments.

timshen marked 2 inline comments as done.Mar 10 2020, 5:43 PM

Address comments.

Harbormaster completed remote builds in B48762: Diff 249526.
bondhugula requested changes to this revision.Mar 10 2020, 7:23 PM

This looks like a good refactoring, thanks! Some (mostly minor) comments.

mlir/include/mlir/Transforms/LoopUtils.h
189

This doc comment should moved down to the method declaration below. Please mention that this performs data copy generation for a single memref region associated with a single op.

191

relevant

191

push -> pushed

198

Nit: Notice -> Note that

203

Doc comment.

207

A better name for 'where'?

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

"At most one copy nest expected" ?

mlir/test/lib/Transforms/TestAffineDataCopy.cpp
42

Test copy generation for a single op / memref region?

This revision now requires changes to proceed.Mar 10 2020, 7:23 PM
timshen updated this revision to Diff 249710.Mar 11 2020, 11:51 AM
timshen marked 8 inline comments as done.

Address comments.

bondhugula added inline comments.Mar 11 2020, 12:41 PM
mlir/include/mlir/Transforms/LoopUtils.h
189

Doc comment here please (even if it's brief).

194

buffer -> buffer allocation

196

alloc above is an operation. alloc -> allocated buffer?

201

generateCopyFromMemRefRegion -> generateCopyForMemRegion ?

215

Camel case.

215

insertionPoint would be used typically for iterator types. Shouldn't this just be a Block::iterator? At the call site, you could pass Block::iterator(op).

mlir/lib/Transforms/Utils/LoopUtils.cpp
1803–1804

Block::iterator(insertion_point) would have given you the iterator. But you don't need this anyway if you pass a Block::iterator itself to this method.

bondhugula added inline comments.Mar 11 2020, 12:45 PM
mlir/test/Transforms/affine-data-copy.mlir
10

Do you need the '=1'? It could be confusing unless it's '=true'.

mlir/test/lib/Transforms/TestAffineDataCopy.cpp
85

/*loopDepth = */0

timshen updated this revision to Diff 249729.Mar 11 2020, 12:54 PM

Update documentation.

bondhugula accepted this revision.Mar 11 2020, 12:55 PM
bondhugula added inline comments.
mlir/include/mlir/Transforms/LoopUtils.h
201

This doc comment appears to provides detail before the actual information. Perhaps just rephrase to:

/// Similar to affineDataCopyGenerate, but works on a single memref region.

You could move "The logic of "find ..." as a comment on the definition of the method.

206

-> a single memref region denoted ...

212

Nit: isn't -> aren't

mlir/test/Transforms/affine-data-copy.mlir
143

// MEMREF-REGION-LABEL

here to be safe.

202

Perhaps a comment here on what is being checked for.

This revision is now accepted and ready to land.Mar 11 2020, 12:55 PM
timshen updated this revision to Diff 249768.Mar 11 2020, 2:57 PM
timshen marked 17 inline comments as done.

Address all comments.

mlir/include/mlir/Transforms/LoopUtils.h
201

Simplified further. The "The logic of" part looks like design rationales rather than function descriptions, which is repeating after the API itself.

215

This op is more than an iterator insertion point. It's also the op that's analyzed by the memref region. It also indicate that something next to the iterator is going to change (std::next(insertionPoint)). Change the name to analyzedOp, and adjusted comments for that.

mlir/lib/Transforms/Utils/LoopUtils.cpp
1803–1804

N/A.

bondhugula accepted this revision.Mar 11 2020, 3:22 PM
This revision was automatically updated to reflect the committed changes.