This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Set presplit attribute in Clang and mlir
ClosedPublic

Authored by ChuanqiXu on Dec 15 2021, 2:52 AM.

Details

Summary

See https://reviews.llvm.org/D100282 and https://github.com/llvm/llvm-project/issues/49264 for the background.
Simply, coroutine shouldn't be inlined before CoroSplit. And the marker for pre-splited coroutine is created in CoroEarly pass, which ran after AlwaysInliner Pass in O0 pipeline. So that the AlwaysInliner couldn't detect it shouldn't inline a coroutine. So here is the error.

This patch set the presplit attribute in clang and mlir. So the inliner would always detect the attribute before splitting.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Dec 15 2021, 2:52 AM
ChuanqiXu requested review of this revision.Dec 15 2021, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 2:52 AM

pass manager changes seem fine, but I'll let somebody else lgtm

llvm/test/Other/new-pm-O0-defaults.ll
34 ↗(On Diff #394501)

should have -NEXT

ChuanqiXu added inline comments.Dec 15 2021, 6:07 PM
llvm/test/Other/new-pm-O0-defaults.ll
34 ↗(On Diff #394501)

In case there is no CHECK-DIS, CHECK-CORO-NEXT doesn't make sense.

rjmccall requested changes to this revision.Dec 15 2021, 10:24 PM

I don't find it at all weird to say that there's an invariant that certain intrinsics can only appear in functions with a particular attribute, and so the frontend has a responsibility to set the attribute if it's using that intrinsic. We do the same thing with several other intrinsics, such as the constrained FP intrinsics generated for #pragma fenv_access.

This revision now requires changes to proceed.Dec 15 2021, 10:24 PM
ChuanqiXu updated this revision to Diff 394801.Dec 16 2021, 2:56 AM
ChuanqiXu retitled this revision from [Coroutines] Run CoroEarly Pass in front of AlwaysInliner in O0 pass and warn for always_inline coroutine to [Coroutines] Set presplit attribute in Clang.
ChuanqiXu edited the summary of this revision. (Show Details)

Address the comments to set the attribute in frontend.

@ezhulenev hi, we are discussing if we should set the coroutine attributes in middle-end or front-end. Now in this revision, it would set the coroutine.presplit attribute in frontend, which would break the MLIR test. I have tried to implement it myself today. But I found that it is really not easy to edit MLIR's code. I found that you introduced the Coroutines intrinsic into MLIR. How do you think about set the attribute in the frontend of MLIR? And if you approve this, could you help to add the attributes in MLIR? We could still add the attribute in CoroEarly pass for a short time to make things consistent.

There are two places where in MLIR you can put an attribute to coroutine functions:

  1. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp#L126

This is the point when coroutine functions are created, and you can attach attribute to the func argument

  1. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp#L325

Another options is to attach attribute to the op->getParentOfType<FuncOp> when async runtime operations lowered to coro.id intrinsic.

/cc @mehdi_amini to chime in what option is better. I'm not sure though what type of MLIR attribute will be translated to LLVM "coroutine.presplit"="0", UnitAttr? Or it must be IntegerAttr.

I'm on vacation with limjted access to computer until the end of the year, I can take a look at it myself ~late Dec or early Jan.

There are two places where in MLIR you can put an attribute to coroutine functions:

  1. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp#L126

This is the point when coroutine functions are created, and you can attach attribute to the func argument

  1. https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp#L325

Another options is to attach attribute to the op->getParentOfType<FuncOp> when async runtime operations lowered to coro.id intrinsic.

/cc @mehdi_amini to chime in what option is better. I'm not sure though what type of MLIR attribute will be translated to LLVM "coroutine.presplit"="0", UnitAttr? Or it must be IntegerAttr.

I'm on vacation with limjted access to computer until the end of the year, I can take a look at it myself ~late Dec or early Jan.

Hi, thanks for looking into this. In this func, https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp#L126, I tried to write:

func.setAttr("coroutine.presplit", "0");

But it shows that we could only set the attribute for argument and result type instead of the function itself. How could we make it?

ChuanqiXu updated this revision to Diff 395835.Dec 22 2021, 3:38 AM
ChuanqiXu retitled this revision from [Coroutines] Set presplit attribute in Clang to [Coroutines] Set presplit attribute in Clang and mlir.
ChuanqiXu edited the summary of this revision. (Show Details)
ChuanqiXu added a reviewer: mehdi_amini.

Set presplit attribute in mlir too

Herald added a project: Restricted Project. · View Herald Transcript

@ezhulenev I finally made it! Thanks for your help! @mehdi_amini @ftynse hi, might you help to look if the change in mlir part is good?

ChuanqiXu updated this revision to Diff 395963.Dec 22 2021, 7:47 PM

Clean codes.

ezhulenev accepted this revision.Dec 24 2021, 8:32 AM
ezhulenev added inline comments.
mlir/lib/Dialect/Async/Transforms/AsyncToAsyncRuntime.cpp
201

nit: builder has an API to simplify attributes construction: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/Builders.h#L92-L100

ChuanqiXu updated this revision to Diff 396254.Dec 26 2021, 6:56 PM

Address comments.

rjmccall added inline comments.Jan 3 2022, 9:58 PM
clang/lib/CodeGen/CGCoroutine.cpp
712

The assertion here is not necessary; if it was, we'd need it everywhere.

Please add a comment like "LLVM expects coroutines to have this attribute set coming out of the frontend."

llvm/docs/Coroutines.rst
1179

Please add this paragraph (modified appropriately) to all of the other coro.id.* intrinsics.

llvm/lib/Transforms/Coroutines/CoroEarly.cpp
198

This should turn into the same assertion as above. (Swift will need a change, but that's Swift's problem, and I'll take care of it.)

llvm/lib/Transforms/Coroutines/CoroInternal.h
42

Editorial: don't put "the" before LLVM here

Since this change requires frontend cooperation, and the refactor will *also* require frontend cooperation, should we just do this in one step so that frontends are less disrupted?

ChuanqiXu updated this revision to Diff 397217.Jan 4 2022, 12:32 AM

Address comments.

rjmccall accepted this revision.Jan 4 2022, 12:36 AM

Alright, that's fine.

This revision is now accepted and ready to land.Jan 4 2022, 12:36 AM
ChuanqiXu marked 2 inline comments as done.Jan 4 2022, 12:39 AM
ChuanqiXu added inline comments.
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
198

I don't add the assertion here since I am worrying it may break some builds. Would you be happy to add the assertion when you change swift?

llvm/lib/Transforms/Coroutines/CoroInternal.h
42

I didn't image swift before when I wrote the FIXME. In case we only need to care about clang and mlir, I think it doesn't matter a lot. But it matters if there is other frontends.
I also found that the values "UNPREPARED_FOR_SPLIT",“PREPARED_FOR_SPLIT” and "ASYNC_RESTART_AFTER_SPLIT" are only meaningful under LegacyPM. @aeubanks , hi what's the status of LegacyPM? Do we need to care about the correctness for the LegacyPM? Or we could give up on it?

This revision was landed with ongoing or failed builds.Jan 4 2022, 6:25 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedJan 4 2022, 8:24 PM

Early heads-up: we see a failure with llvm/lib/Transforms/Coroutines/CoroEarly.cpp:186 in bool (anonymous namespace)::Lowerer::lowerEarlyIntrinsics(llvm::Function &): F.hasFnAttribute(CORO_PRESPLIT_ATTR) && F.getFnAttribute(CORO_PRESPLIT_ATTR).getValueAsString() == UNPREPARED_FOR_SPLIT && "The frontend uses Swtich-Resumed ABI should emit " "\"coroutine.presplit\" attribute with value \"0\" for the " "coroutine."

A colleague may follow up with the issue or request a revert.

Early heads-up: we see a failure with llvm/lib/Transforms/Coroutines/CoroEarly.cpp:186 in bool (anonymous namespace)::Lowerer::lowerEarlyIntrinsics(llvm::Function &): F.hasFnAttribute(CORO_PRESPLIT_ATTR) && F.getFnAttribute(CORO_PRESPLIT_ATTR).getValueAsString() == UNPREPARED_FOR_SPLIT && "The frontend uses Swtich-Resumed ABI should emit " "\"coroutine.presplit\" attribute with value \"0\" for the " "coroutine."

A colleague may follow up with the issue or request a revert.

Thanks for reporting this. I have tested this locally with our workloads. I guess there are 3 reasons to hit this:
(1) The frontend (clang or mlir) is updated.
(2) The input IR is generated by an old frontend.
(3) The tested frontend uses switched-resume ABI coroutine intrinsics and the frontend isn't clang nor mlir.