Page MenuHomePhabricator

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

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



As discussed in 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

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.


80 cols plz


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


Nit: make static + add documentation.


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.


Why limit to the first result only?


Nit: /// for top-level comments.


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


This pattern needs dedicated tests.


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.


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.

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.

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


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


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


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