Page MenuHomePhabricator

[Coroutines] Set presplit attribute in Clang instead of CoroEarly pass
AcceptedPublic

Authored by lxfind on Apr 11 2021, 10:07 PM.

Details

Summary

Presplit coroutines cannot be inlined. During AlwaysInliner we check if a function is a presplit coroutine, if so we skip inlining.
The presplit coroutine attributes are set in CoroEarly pass.
However in O0 pipeline, AlwaysInliner runs before CoroEarly, so the attribute isn't set yet and will still inline the coroutine.
This causes Clang to crash: https://bugs.llvm.org/show_bug.cgi?id=49920

To fix this, we set the attributes in the Clang front-end instead of in CoroEarly pass.

Diff Detail

Event Timeline

lxfind created this revision.Apr 11 2021, 10:07 PM
lxfind requested review of this revision.Apr 11 2021, 10:07 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 11 2021, 10:07 PM

Why does this pass even exist? We should just expect the frontend to set the attribute. It's not like frontends don't have to otherwise know that they're emitting a coroutine; a ton of things about the expected entire IR pattern are different.

Why does this pass even exist? We should just expect the frontend to set the attribute. It's not like frontends don't have to otherwise know that they're emitting a coroutine; a ton of things about the expected entire IR pattern are different.

The attribute setting can totally be moved to the front-end.
One thing that's not clear to me is whether we should simply set coroutine functions noinline instead of replying on the attributres for this.
GCC seems to complain about inlining coroutines: https://godbolt.org/z/KrzE1znno, not fully sure why.

As for the CoroEarly pass, it lowers a bunch of intrinsics. Technically I think they can all be done in the front-end. But moving some complexity out of front-end to IR seems reasonable to me.

Why does this pass even exist? We should just expect the frontend to set the attribute. It's not like frontends don't have to otherwise know that they're emitting a coroutine; a ton of things about the expected entire IR pattern are different.

The attribute setting can totally be moved to the front-end.
One thing that's not clear to me is whether we should simply set coroutine functions noinline instead of replying on the attributres for this.
GCC seems to complain about inlining coroutines: https://godbolt.org/z/KrzE1znno, not fully sure why.

Well, *properly* inlining a coroutine requires merging the functions in ways that LLVM's representation wouldn't really support, and I assume that's true of GCC's representation as well. The best we can do is lower the coroutine and then inline the ramp function, which is not really the same thing. So warning about marking a coroutine always_inline does make some sense.

As for the CoroEarly pass, it lowers a bunch of intrinsics. Technically I think they can all be done in the front-end. But moving some complexity out of front-end to IR seems reasonable to me.

Ah, if the pass does more than just setting the attribute, then sure, it makes sense to keep it. But I do think we should be requiring the attribute to be added by frontends, since it's really an IR invariant that it's present on all unlowered coroutines.

This patch looks OK to me.

The best we can do is lower the coroutine and then inline the ramp function, which is not really the same thing. So warning about marking a coroutine always_inline does make some sense.

It makes sense to emit a warning instead of an error. The example above emits an error which is surprising.

Ah, if the pass does more than just setting the attribute, then sure, it makes sense to keep it. But I do think we should be requiring the attribute to be added by frontends, since it's really an IR invariant that it's present on all unlowered coroutines.

By the way, it also sets these attributes for other types of coroutines (retcon and async). So the down-side would be then we need to do this for all front-ends (clang and swift).

Ah, if the pass does more than just setting the attribute, then sure, it makes sense to keep it. But I do think we should be requiring the attribute to be added by frontends, since it's really an IR invariant that it's present on all unlowered coroutines.

By the way, it also sets these attributes for other types of coroutines (retcon and async). So the down-side would be then we need to do this for all front-ends (clang and swift).

If it is OK to add these attributes in clang only, and leave other codes handling retcon and async in the Coro-early pass? Since they use special intrinsics, it looks good if we don't edit corresponding part in Coro-early pass.

We don't have so many coroutine-emitting frontends that it's unreasonable to modify them all. Swift can make this change; it's not on you to do it. (I also work on the Swift frontend, so don't worry, I'm signing my own teammates up for work here.)

lxfind updated this revision to Diff 336850.Apr 12 2021, 8:46 AM

Set the attributes in Clang instead of CoroEarly

lxfind retitled this revision from [Coroutines] Move CoroEarly pass to before AlwaysInliner to [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.Apr 12 2021, 8:47 AM
lxfind edited the summary of this revision. (Show Details)
lxfind updated this revision to Diff 337259.Apr 13 2021, 2:17 PM

Update test

This revision is now accepted and ready to land.Apr 13 2021, 2:38 PM
ychen added a subscriber: ychen.Apr 13 2021, 7:21 PM

I think the setting is in CoroEarly from the beginning is that it is an implementation detail? Clients should only worry about coroutine shape. Maybe we could set noinline in frontends to express the intent and remove it in coroearly/corosplit?

That something is an (unlowered) coroutine is an important semantic property of the function that should be represented more explicitly in IR than just whether it contains a call to an intrinsic in the llvm.coro.id family. Coroutines have somewhat different structural rules from ordinary functions, as is natural for a somewhat different high-level concept. noinline is an independent attribute that should affect whether the ramp function is inlinable after coroutine lowering is complete.

In principle, I would prefer that we use a better attribute than this coro.presplit thing — a few years ago, I proposed adding a directly-supported coroutine attribute which would take the lowering style as an argument. So if someone's willing to do to the work to jump to that instead of going through this intermediate stage, then okay, let's do it.

I think the setting is in CoroEarly from the beginning is that it is an implementation detail? Clients should only worry about coroutine shape. Maybe we could set noinline in frontends to express the intent and remove it in coroearly/corosplit?

We cannot do that, because we need to distinguish between user-specified noinline vs coroutine. Some coroutines in theory could potentially be inlined.
Our choice is really just between setting it in the front-end or moving CoroEarly to the beginning of the pipeline.

ChuanqiXu accepted this revision.Apr 13 2021, 8:33 PM

I agree with the idea we should set the attribute in the frontend or move Coro-early pass in the front. It looks more weird to add a noinline attribute and move it later.

This revision was landed with ongoing or failed builds.Apr 18 2021, 2:57 PM
This revision was automatically updated to reflect the committed changes.

This broke MLIR tests.
It seems that MLIR tests depend on CoroEarly to be able to annotate coroutine function properly based on the intrinsics.
Given that, I am now convinced we shouldn't set the attribute in the frontend. Instead we should simply move CoroEarly to before AlwaysInliner.

MLIR is an in-tree project that can be updated.

MLIR is an in-tree project that can be updated.

Sure, but I think there are some important differences.
As far as I understand, in MLIR, unlike in C++/Swift frontend where a coroutine function body is represented by an explicit AST type, there is no concept for coroutine functions. Instead functions just contain async dialects. So for MLIR to properly annotate coroutine functions, it will need to look for either those dialects or these intrinsics after IRGen in order to do so, which is pretty much the same thing that we were doing in CoroEarly to annotate coroutine functions. The complexity introduced by duplicating this to all frontends, especially in MLIR where we need to do the same thing as we were doing in CoroEarly, seems to out-weight the benefits on conceptual clarity.

lxfind reopened this revision.Tue, Apr 20, 9:51 AM
This revision is now accepted and ready to land.Tue, Apr 20, 9:51 AM