Page MenuHomePhabricator

[coroutine] should disable inline before calling coro split
ClosedPublic

Authored by lxfind on Dec 4 2020, 4:12 PM.

Details

Summary

This is a rework of D85812, which didn't land.
When callee coroutine function is inlined into caller coroutine function before coro-split pass, llvm will emits "coroutine should have exactly one defining @llvm.coro.begin". It seems that coro-early pass can not handle this quiet well.
So we believe that unsplited coroutine function should not be inlined.
This patch fix such issue by not inlining function if it has attribute "coroutine.presplit" (it means the function has not been splited) to fix this issue
test plan: check-llvm, check-clang

In D85812, there was suggestions on moving the macros to Attributes.td to avoid circular header dependency issue.
I believe it's not worth doing just to be able to use one constant string in one place.
Today, there are already 3 possible attribute values for "coroutine.presplit": https://github.com/llvm/llvm-project/blob/c6543cc6b8f107b58e7205d8fc64865a508bacba/llvm/lib/Transforms/Coroutines/CoroInternal.h#L40-L42
If we move them into Attributes.td, we would be adding 3 new attributes to EnumAttr, just to support this, which I think is an overkill.

Instead, I think the best way to do this is to add an API in Function class that checks whether this function is a coroutine, by checking the attribute by name directly.

Diff Detail

Event Timeline

lxfind created this revision.Dec 4 2020, 4:12 PM
lxfind requested review of this revision.Dec 4 2020, 4:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 4:12 PM

I believe it's not worth doing just to be able to use one constant string in one place.
If we move them into Attributes.td, we would be adding 3 new attributes to EnumAttr, just to support this, which I think is an overkill.

Agreed that string is fine. There're many precedences where string is used in getFnAttribute/hasFnAttribute already, otherwise we will need to be consistent.

llvm/include/llvm/IR/Function.h
272

What is the definition of a coroutine function here? The main coroutine function alone, or does that include split funclets? A function is consider a coroutine function only before corosplit, but no longer after split?

I'm not sure if presplit attribute is the best way to tell whether function is coroutine because the attribute is added, removed in the process of codegen only to reflect split state. And it doesn't look like an invariant. Also what we need here in the context of inline check is not whether a function is coroutine, but wether a function is a presplit coroutine, right?

llvm/lib/Analysis/InlineCost.cpp
2339

This works with the current definition of isCoroutine which is based on split. But the naming is misleading because it may lead people to think that we can't inline any coroutine.

dongAxis1944 added inline comments.Dec 6 2020, 6:39 PM
llvm/include/llvm/IR/Function.h
272

I do not think it is the best way to use the string coroutine.presplit instead of the macro CORO_PRESPLIT_ATTR.

The other parts in LLVM uses CORO_PRESPLIT_ATTR, but not directly use the constant string. so I think it might keep it same.

lxfind added inline comments.Dec 7 2020, 8:18 AM
llvm/include/llvm/IR/Function.h
272

@dongAxis1944 Where would you suggest to put the definition of CORO_PRESPLIT_ATTR? From what I can see, it seems common in LLVM to use string attribute directly (not saying it's good, but there doesn't seem to be an infrastructure to make this better).

lxfind updated this revision to Diff 309927.Dec 7 2020, 8:33 AM

Add missing file. Rename isCoroutine to isPresplitCoroutine

junparser accepted this revision.Dec 8 2020, 12:13 AM

hi @lxfind, sorry for the late reply. After D85812, we think it might be better to move both of attribute and intrinsic of coroutine into core library, however such changes are too heavy to do.
The reason I suggest adding the attribute into EnumAttr is that i believe that we may need to use presplit attribute in other places (such as lower debug intrinsic. ) and we do not want to write the string anywhere cross different parts. Add API in Function will reduce the impact, so LGTM, thanks for the patch.

This revision is now accepted and ready to land.Dec 8 2020, 12:13 AM
This revision was automatically updated to reflect the committed changes.
aeubanks added inline comments.
llvm/test/Transforms/Coroutines/coro-inline.ll
1

Are these -barriers necessary? The test passes without them. And no other test in all of LLVM uses it.

2

-enable-new-pm shouldn't be used to run the NPM, this should be something like -passes=always-inline,coro-early,coro-split.

32

seems like this test should be reduced, is any of this debug info metadata necessary for the test?

lxfind added inline comments.Dec 10 2020, 8:29 AM
llvm/test/Transforms/Coroutines/coro-inline.ll
1

@dongAxis1944, do you know the context on why this was initially added?

dongAxis1944 added inline comments.Dec 13 2020, 9:58 PM
llvm/test/Transforms/Coroutines/coro-inline.ll
1

this is because we want to run always_inline, then coro-early, split at last. using barrier, will keep this sequence well.

aeubanks added inline comments.Dec 13 2020, 10:28 PM
llvm/test/Transforms/Coroutines/coro-inline.ll
1

Does it matter if pass A runs on all SCCs, then pass B runs on all SCCs, or if for each SCC, pass A runs then pass B runs before going to the next SCC?

dongAxis1944 added inline comments.Dec 13 2020, 11:03 PM
llvm/test/Transforms/Coroutines/coro-inline.ll
1

the actual actions is that llvm will do always inline for all scc, before doing early and split, so we add barrier.