This is an archive of the discontinued LLVM Phabricator instance.

[SCF][MemRef] Enable SCF.Parallel Lowering to use Scope Op
ClosedPublic

Authored by wsmoses on Feb 23 2022, 11:12 AM.

Details

Summary

As discussed in https://reviews.llvm.org/D119743 scf.parallel would continuously stack allocate since the alloca op was placd in the wsloop rather than the omp.parallel. This PR is the second stage of the fix for that problem. Specifically, we now introduce an alloca scope around the inlined body of the scf.parallel and enable a canonicalization to hoist the allocations to the surrounding allocation scope (e.g. omp.parallel).

Diff Detail

Event Timeline

wsmoses created this revision.Feb 23 2022, 11:12 AM
wsmoses requested review of this revision.Feb 23 2022, 11:12 AM
mehdi_amini added inline comments.Feb 23 2022, 11:32 AM
mlir/lib/Conversion/SCFToOpenMP/SCFToOpenMP.cpp
398–418

Please run clang-format?

wsmoses updated this revision to Diff 410889.Feb 23 2022, 11:37 AM

Clang format

ftynse requested changes to this revision.Feb 24 2022, 2:10 AM

Conversion changes look okay. Canonicalization changes less so. An explicit allocation scope forces deallocation at its end, and it looks like both canonicalizations would change the point where deallocation happens. It's okay to merge two nested allocation scopes if they are perfectly nested, otherwise the allocations from the nested scope will be live longer than before. Similarly, hoisting can change the lifetime of an allocation. I'm fine with such lifetime-extending transformations in a separate pass, but not as canonicalizations (which are not expected to change semantics).

Specifically about the hoisting pattern, I don't completely follow what it does and there are no tests specifically for it. It seems to be moving allocations from a scope operation a parent scope operation, which may not be always desirable.

mlir/include/mlir/Conversion/Passes.td
504–505

80 cols plz

mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
2601

Let's not spuriously move the code around. Op implementations were sorted alphabetically AFAICS.

2656

Nit: make static + add documentation.

2657–2659

This is too optimistic. If the operation, e.g., an unregistered one, doesn't implement the interface, it may still perform the allocation. We can only be sure that it does not allocate if it has the interface and does not declare the effect with it.

2661

Why limit to the first result only?

2669

Nit: /// for top-level comments.

2677–2681

Could you extract the lambda or the walk from the if?

2696

This pattern needs dedicated tests.

2701–2709

This makes assumptions about the nesting structure of ops. In particular, it seems to assume there is always a parent op with the scope trait. This is not necessarily the case, e.g., one can have a custom function op that doesn't have the trait for whatever reason. Please check that the parent op exists before querying its properties.

2721–2722

You shouldn't be needing to iterate over all regions of a parent op, you know which one of those contains the current op.

This revision now requires changes to proceed.Feb 24 2022, 2:10 AM
wsmoses marked 8 inline comments as done.Feb 24 2022, 8:17 AM
wsmoses added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
2696

The two region-based scopeMerge2 and scopeMerge3 tests should test this canonicalizer (and ensure it does and/or doesn't run when relevant).

wsmoses updated this revision to Diff 411138.Feb 24 2022, 8:21 AM

Address comments

ftynse accepted this revision.Mar 1 2022, 3:29 AM
ftynse added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
296–298

I can't parse the "last non terminating block in a region", blocks are not terminators, ops are.

353

if (lastParentWithoutScope == nullptr) return failure();

362–363

while (lastParentWIthoutScope->getParentOp() && the-curent-condition) or factor it out to a variable

mlir/test/Dialect/MemRef/canonicalize.mlir
567

Nit: we likely don't care about these operations being on consecutive lines, so no need for -NEXT here and below.

This revision is now accepted and ready to land.Mar 1 2022, 3:29 AM
wsmoses updated this revision to Diff 412142.Mar 1 2022, 9:50 AM
wsmoses marked 2 inline comments as done.

Address comments