This is an archive of the discontinued LLVM Phabricator instance.

[Pipelines] Remove Legacy Passes in Coroutines
ClosedPublic

Authored by ChuanqiXu on Apr 17 2022, 8:20 PM.

Details

Summary

I found there are some patches which removed legacy passes. So I am wondering if we could remove the legacy passes in coroutines now. So that we could simplify the codes further.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Apr 17 2022, 8:20 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: hiraditya. · View Herald Transcript
ChuanqiXu requested review of this revision.Apr 17 2022, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2022, 8:20 PM
ChuanqiXu edited the summary of this revision. (Show Details)Apr 17 2022, 8:23 PM
ChuanqiXu added reviewers: aeubanks, rjmccall, MaskRay.
ChuanqiXu set the repository for this revision to rG LLVM Github Monorepo.
ChuanqiXu retitled this revision from [Pipelines] Remove Legacy Passes in to [Pipelines] Remove Legacy Passes in Coroutines.Apr 18 2022, 1:07 AM
nikic added a subscriber: nikic.Apr 18 2022, 1:08 AM
nikic added inline comments.
llvm/tools/opt/opt.cpp
374

You can also drop the option now. It is only meaningful with legacy PM, as coroutines are always enabled with new PM.

ChuanqiXu updated this revision to Diff 423346.Apr 18 2022, 1:23 AM

Address comments and fix tests.

ChuanqiXu marked an inline comment as done.Apr 18 2022, 1:23 AM
ChuanqiXu added inline comments.
llvm/test/Transforms/Coroutines/coro-async-addr-lifetime-start-bug.ll
1–2

I am wondering if the 2 tests are redundant?

nikic added inline comments.Apr 18 2022, 1:36 AM
llvm/test/Transforms/Coroutines/coro-async-addr-lifetime-start-bug.ll
1–2

Yes, these are redundant, you can drop one of them.

ChuanqiXu updated this revision to Diff 423348.Apr 18 2022, 1:47 AM

Address comments.

ChuanqiXu marked an inline comment as done.Apr 18 2022, 1:47 AM
aeubanks accepted this revision.Apr 18 2022, 9:57 AM
This revision is now accepted and ready to land.Apr 18 2022, 9:57 AM
MaskRay added a comment.EditedApr 18 2022, 10:46 AM

I found there are some patches which removed legacy passes.

Yes. I have sent some after I saw nikic did for some.

An important user of us swiftshader uses createCoroCleanupLegacyPass. Would you mind giving us few days so that they can migrate away?

I found there are some patches which removed legacy passes.

Yes. I have sent some after I saw nikic did for some.

An important user of us swiftshader uses createCoroCleanupLegacyPass. Would you mind giving us few days so that they can migrate away?

Of course!

I found there are some patches which removed legacy passes.

Yes. I have sent some after I saw nikic did for some.

An important user of us swiftshader uses createCoroCleanupLegacyPass. Would you mind giving us few days so that they can migrate away?

Of course!

We've fixed up the user, feel free to commit this patch.

This revision was landed with ongoing or failed builds.Apr 20 2022, 8:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2022, 8:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
llvm/include/llvm-c/Transforms/Coroutines.h