Page MenuHomePhabricator

[MLIR] Extend isLoopMemoryParallel to account for locally allocated memrefs
ClosedPublic

Authored by bondhugula on Feb 26 2022, 7:23 AM.

Details

Summary

Extend isLoopMemoryParallel check to include locally allocated memrefs.
This strengthens and also speeds up the dependence check used by the
utility by excluding locally allocated memrefs where appropriate.

Additional memref dialect ops can be supported exhaustively via proper
interfaces.

Diff Detail

Event Timeline

bondhugula created this revision.Feb 26 2022, 7:23 AM
bondhugula requested review of this revision.Feb 26 2022, 7:23 AM

Fix test case spacing.

Thanks, Uday. A couple of comments that would need attention. LGTM. You can move forward after addressing them.

mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
122

Use Operation::IsAncestor instead?

146–149

I think it's important that we move this information to the utility documentation above. We may also want to rename it so that the name state that it only checks for memory dependences. I would expect isLoopMemoryParallel to also check for memrefs that escape or used as iter args.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 9:37 AM
dcaballe accepted this revision.Mar 2 2022, 9:37 AM
This revision is now accepted and ready to land.Mar 2 2022, 9:37 AM
ayzhuang added inline comments.Mar 2 2022, 5:14 PM
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
137

Do we need isNestedWithin? How about !isDefinedOutsideOfLoop since enclosingOp is essentially AffineForOp?

bondhugula marked 2 inline comments as done.Mar 2 2022, 6:07 PM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
122

Thanks! isProperAncestor is the one that I missed using here.

137

The isNestedWithin is the same as "not being defined outside of loop". It's defined inside the loop if and only if enclosingOp is a proper ancestor of defOp. Any situation you had in mind that's not covered?

146–149

I think it's important that we move this information to the utility documentation

I kept the detailed comment here and added an additional line on the util doc comment clarifying that even memrefs on iter_args aren't checked.

We may also want to rename it so that the name state that

This is a good point. I was thinking about this. The current doc comment already states:

This function doesn't check iter_args and should be used
only as a building block for full parallel-checking functions.

How about actually checking for memref types being passed as iter_args in this utility? That way, all memory dependences would be accounted for and the name isLoopMemoryParallel would be accurate and it would still be conservatively detecting parallel loops irrespective of any memref typed iter_args. (You have the other method isLoopParallel that gives the semblances of a full check given the latter's general name.)

bondhugula updated this revision to Diff 412585.Mar 2 2022, 6:08 PM
bondhugula marked 2 inline comments as done.

Address some of the review comments.

bondhugula added inline comments.Mar 2 2022, 6:16 PM
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
146–149

Added an extra to doc comment that memrefs allocated inside forOp don't impact dependences and parallelism. While we can add a check for memrefs among iter arguments, it doesn't defend against custom/unknown types (say out-of-tree types) that somehow alias with memrefs -- but the latter part should be clear since "memory" for this method is associated with built-in memrefs. isLoopParallel is expected to take care of all iter_args.

Hardcode84 added inline comments.
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
134

Use ViewLikeOpInterface?

bondhugula updated this revision to Diff 412696.Mar 3 2022, 6:25 AM
bondhugula marked an inline comment as done.

Use ViewLikeOpInterface.

mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
134

Thanks - this was what I was missing!

ayzhuang added inline comments.Mar 3 2022, 8:21 AM
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
137

No, I meant that we didn't need to define a new function.

dcaballe accepted this revision.Mar 3 2022, 12:11 PM
dcaballe added inline comments.
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
146–149

I guess it's ok as long as it's properly documented.

How about actually checking for memref types being passed as iter_args in this utility?

That sounds good to me and I would also consider adding checks for memrefs that escape. That would keep all the memory checks in one place and would make isLoopMemoryParallel less surprising to the user, I think. However, I'm ok with the current implementation as long as it is properly documented.

bondhugula marked 2 inline comments as done.Mar 3 2022, 7:36 PM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Analysis/AffineAnalysis.cpp
146–149

Done. (Note that since the result types of the affine.for match 1:1 with iter_args/iter operands, just checking for memref types in result types is sufficient to check for both cross-iter memref flow and escapes out of loop.)

bondhugula updated this revision to Diff 412903.Mar 3 2022, 7:36 PM
bondhugula marked an inline comment as done.

Check iter_args for memref types.

bondhugula updated this revision to Diff 412906.Mar 3 2022, 7:40 PM

Adjust comments and include.

bondhugula updated this revision to Diff 412907.EditedMar 3 2022, 7:43 PM

Rebase and resolve conflict.

bondhugula updated this revision to Diff 412909.Mar 3 2022, 7:49 PM

Trim extra include.

This revision was landed with ongoing or failed builds.Mar 3 2022, 7:50 PM
This revision was automatically updated to reflect the committed changes.