Page MenuHomePhabricator

[coroutine] should disable inline before calling coro split
ClosedPublic

Authored by dongAxis1944 on Aug 11 2020, 11:42 PM.

Details

Summary

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

Diff Detail

Event Timeline

dongAxis1944 created this revision.Aug 11 2020, 11:42 PM
dongAxis1944 requested review of this revision.Aug 11 2020, 11:42 PM
dongAxis1944 added a reviewer: junparser.
dongAxis1944 edited the summary of this revision. (Show Details)
dongAxis1944 retitled this revision from [CORO] should disable inline before calling coro split to [coroutine] should disable inline before calling coro split.
dongAxis1944 edited the summary of this revision. (Show Details)
dongAxis1944 added reviewers: junparser, wenlei, mtrofin.
dongAxis1944 edited the summary of this revision. (Show Details)Aug 12 2020, 11:01 PM

The "repository" of this change seems incorrect. It should be "rG LLVM Github Monorepo", instead it's "rZORG LLVM Github Zorg". I wonder if that's why the context of the changes isn't available?

update new diff for showing the context of the changes

The "repository" of this change seems incorrect. It should be "rG LLVM Github Monorepo", instead it's "rZORG LLVM Github Zorg". I wonder if that's why the context of the changes isn't available?

sorry for that, i forgot to commit context of the changed.

You'd also want to prevent inlining presplit coroutine in sample profile loader (SampleProfileLoader::inlineHotFunctions), which is before CGSCC pipeline and does top-down inlining now.

llvm/lib/Analysis/InlineCost.cpp
2400

I can see the problem with always inliner as that is outside of CGSCC pipeline, and caller could be processed before callee. However, do you actually run into this problem with CGSCC inliner? I'd expect callee to be split already as they should be processed earlier in the pipeline.

wenlei added a subscriber: davidxl.Aug 15 2020, 6:11 PM

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

yes, my change in 'llvm::getInlineCost' was for sample profile loader inline, not for CGSCC. I think you might be right, i will do my change in always inline and sample profile loader inline. Thank you for your advice.

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

Yep. Since corosplit pass runs before CGSCC inline, this should not happen for CGSCC inline.

move the check from llvm::getInlineCost to SampleProfileLoader::inlineHotFunctions and AlwaysInlinerLegacyPass::getInlineCost

wenlei added inline comments.Aug 17 2020, 9:14 PM
llvm/lib/Transforms/IPO/SampleProfile.cpp
1072 ↗(On Diff #286192)

Move this into inlineCallInstruction? inlineCallInstruction has another call site on line 1059. Would be good to add a comment too, for sample loader and always inline.

dongAxis1944 marked an inline comment as done.

move check to SampleProfileLoader::inlineCallInstruction

wenlei accepted this revision.Aug 18 2020, 7:21 AM

LGTM, thanks for the fix.

This revision is now accepted and ready to land.Aug 18 2020, 7:21 AM
MaskRay added a subscriber: MaskRay.EditedAug 24 2020, 11:31 AM

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

MaskRay added inline comments.Aug 24 2020, 11:36 AM
llvm/test/Transforms/Coroutines/coro-inline.ll
2

You may consider split-file %s %t to place sample.text.prof in this file.

MaskRay added inline comments.Aug 24 2020, 11:55 AM
llvm/include/llvm/Transforms/Coroutines.h
24

The code motion was not needed, I guess?

It probably should have used constexpr instead of macros...

hoy added a subscriber: hoy.Aug 25 2020, 11:04 PM

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

if i put the attribute CORO_PRESPLIT_ATTR to the attribute.td, it seem it might not have the layering issue(Because it will not depend coroutine lib itself), do you think it might be better?

I want to point out a strange layering issue. Note that the test llvm/test/Transforms/Coroutines/coro-inline.ll uses -sample-profile: this implies that Coroutines should be a higher level library than sample-profile (a higher level library can use all facilities available at lower layers). However, LLVMipo (the library which contains sample-profile) has a dependency on LLVMCoroutines (the library which contains Coroutines.h)....... This is a conflict of library layering.

edit: LLVMCoroutines actually depends on LLVMipo, so circular dependency

You're right about the circular dependency on headers. Unlike actual circular dependency on actual libs though, such layering violation from header is not obvious, curious how did you catch this?

@dongAxis1944 to avoid the layering issues, I think you may need to revert back to your original version that prohibits inlining of pre-split coroutine in the general inline cost computation. The down side of that is we might cover up pass ordering issue in the future, but that's minor comparing the layering issue.

if i put the attribute CORO_PRESPLIT_ATTR to the attribute.td, it seem it might not have the layering issue(Because it will not depend coroutine lib itself), do you think it might be better?

For attribute.td, it will be automatically checked for inlining through hasCompatibleFnAttrs. The CompatRule<"isEqual .. rule checks whether the caller and callee has the same attribute value, which may be different from what you need. There maybe a way to write a compatible rule that fits your need? I'm not familiar with how to do that though (or if that's possible)..

dongAxis1944 added a reviewer: MaskRay.

hello, i just update previous patch, if it is ok, i will commit the code

dongAxis1944 added inline comments.Sep 1 2020, 6:57 AM
llvm/include/llvm/Transforms/Coroutines.h
24

yes, but i want to use another patch to resolve it. We prepare to move this constant to attribute.td

dongAxis1944 added inline comments.Sep 1 2020, 6:58 AM
llvm/test/Transforms/Coroutines/coro-inline.ll
2

how to do it? do you have any examples?

MaskRay added inline comments.Sep 1 2020, 9:20 AM
llvm/test/Transforms/Coroutines/coro-inline.ll
2

Search for split-file %s in llvm/test. There are many examples.

Took another look, AlwaysInline is part of LLVMipo too. :(

To abide the layering, this attribute check for AlwaysInline also needs to be in InlineCost, which is in Analysis lib. I think legality checks should be shared between SCC Inline and Always Inline, but that'd be a refactoring by itself. Moving it to attribute.td is another way to do it (need to make sure hasCompatibleFnAttrs works properly). If you plan to move it to attribute.td always, it might make sense to combine the change so layering isn't an issue for this change.

On the other hand, I do see string literal being used for string attributes (example: "no-builtins")...

llvm/test/Transforms/Coroutines/coro-inline.ll
2

You may consider split-file %s %t to place sample.text.prof in this file.

I would prefer to keep sample.text.prof in a separate file as is, so the file itself is a legal profile. This is also more consistent with how other test cases use profile as input.

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

Hi @dongAxis1944, @junparser, do you still plan to reland this patch?

For now,we do not have time to refactor this. For this case, we can move all attributes of coroutine to LLVMCore library. If you do have anytime, feel free to do this.