Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Coroutine][DebugInfo] Update the linkage name of the declaration of coro-split functions in the debug info. #7168

Merged
merged 3 commits into from
Sep 1, 2023
Merged

[Coroutine][DebugInfo] Update the linkage name of the declaration of coro-split functions in the debug info. #7168

merged 3 commits into from
Sep 1, 2023

Conversation

DianQK
Copy link

@DianQK DianQK commented Aug 8, 2023 •
edited

Differential Revision: https://reviews.llvm.org/D157184

(cherry picked from commit ca1a5b3)
(cherry picked from commit 88a83c9)
(cherry picked from commit 3aed002)

cc @adrian-prantl

@DianQK
[Coroutine][DebugInfo] Pre-commit test for a DISubprogram with declar…
c761d7f 
…ation. (NFC)

Pre-commit test for D157184.

Differential Revision: https://reviews.llvm.org/D157177

(cherry picked from commit 88a83c9)
@DianQK
[Coroutine][DebugInfo] Update the linkage name of the declaration of …
03e3219 
…coro-split functions in the debug info.

This patch adds the linkage name update to DISubprogram's declaration after 6ce76ff.

Reviewed By: ChuanqiXu

Differential Revision: https://reviews.llvm.org/D157184

(cherry picked from commit ca1a5b3)
@adrian-prantl
Copy link
Member

@swift-ci test

@felipepiovezan
Copy link

The cherry-picks LGTM. I did leave a comment in the original review regarding how the test was created; it would be nice to address that upstream!

@DianQK
Copy link
Author

DianQK commented Aug 16, 2023

The cherry-picks LGTM. I did leave a comment in the original review regarding how the test was created; it would be nice to address that upstream!

Thanks. I saw it. I will try to reduce this test again.

@DianQK
[Coroutine][DebugInfo] Reduced test case for `coro-async-declaration.…
50f53a4 
…ll`. (NFC)

Based on the description at https://reviews.llvm.org/D157177#inline-1529005, I try to improve the test case.

Reviewed By: fdeazeve

Differential Revision: https://reviews.llvm.org/D158178

(cherry picked from commit 3aed002)
@DianQK
Copy link
Author

DianQK commented Aug 17, 2023

I picked the new test from https://reviews.llvm.org/D158178.
The upstream commit is llvm@3aed002.

@DianQK
Copy link
Author

DianQK commented Aug 31, 2023

Gently pinging.

@felipepiovezan
Copy link

Hi @DianQK ,
The PR is already approved, but we're missing a test run after the new push
@swift-ci test

@DianQK
Copy link
Author

DianQK commented Sep 1, 2023

I don't have access to @swift-ci. So I'll just have to wait for you to invoke the test.
The failure now looks like a ci script error?

@felipepiovezan
Copy link

@swift-ci test

@felipepiovezan
Copy link

I don't have access to @swift-ci. So I'll just have to wait for you to invoke the test.

Apologies, I wasn't aware of that!

The failure now looks like a ci script error?

Mmmm, let's try that again.

@felipepiovezan felipepiovezan merged commit 3d4ba0c into apple:stable/20221013 Sep 1, 2023
3 checks passed
@DianQK DianQK deleted the stable/20221013 branch September 1, 2023 21:17
@DianQK
Copy link
Author

DianQK commented Sep 1, 2023

Thank you very much, I think the apple/swift#67077 test should pass as well.
Could you run the test again？

@felipepiovezan
Copy link

All merged!

@DianQK
Copy link
Author

DianQK commented Sep 6, 2023

Thanks!

@drodriguez
Copy link

I think for that test to work opt might need support for the memory(…) attributes introduced in https://reviews.llvm.org/D135780 which are not part of the stable/20221013 branch. There might be other pieces needed, I am not sure. It might be easier to go back to the previous spelling of the attributes in the test, but since stable/20230725 is going to happen soon-ish, that might mean problems later.

@DianQK
Copy link
Author

DianQK commented Sep 8, 2023

I think for that test to work opt might need support for the memory(…) attributes introduced in https://reviews.llvm.org/D135780 which are not part of the stable/20221013 branch. There might be other pieces needed, I am not sure. It might be easier to go back to the previous spelling of the attributes in the test, but since stable/20230725 is going to happen soon-ish, that might mean problems later.

Weirdly, the CI has already passed. Do you have any suggestions?

@drodriguez
Copy link

As far as I understand, CI only runs the tests of the Swift compiler, so it only checks that changes to LLVM do not break the Swift compiler, but the LLVM/Clang test suites are not executed.

@DianQK
Copy link
Author

DianQK commented Sep 11, 2023

It might be easier to go back to the previous spelling of the attributes in the test, but since stable/20230725 is going to happen soon-ish, that might mean problems later.

Maybe the documentation is out of date, or I'm looking in the wrong place. I'm not sure about the apple/llvm-project branching model. I think stable/20221013 should be merged to stable/20230725. if we do nothing, for since stable/20230725 development will not encounter problems.

Since apple fork project are generally not expected to have no upstream commits. I'm not sure how to do it properly.
Commit a change and then revert it in stable/20230725?

@felipepiovezan
Copy link

I think stable/20221013 should be merged to stable/20230725.

I believe this is not correct. stable/20230725 was branched from upstream LLVM at 2023-07-25. If the commits in this PR were also placed upstream prior to that date, then they are also in stable/20230725.

Did this PR break any tests inside llvm-project? If this PR is relying on a feature that does not exist in opt in stable/20221013 but it exists in stable/20230725, then we should fix the test only in stable/20221013

@DianQK
Copy link
Author

DianQK commented Sep 11, 2023

Did this PR break any tests inside llvm-project?

According to @drodriguez's description, the coro-async-declaration.ll test should fail because memory(argmem: readwrite) is not available.

I believe this is not correct. stable/20230725 was branched from upstream LLVM at 2023-07-25. If the commits in this PR were also placed upstream prior to that date, then they are also in stable/20230725.

Thanks, I get it, great. This change landed in August.
So I should remove memory(*) in stable/20221013. Then cherry pick these commits in stable/20230725.

@drodriguez
Copy link

Did this PR break any tests inside llvm-project?

In our test infra it does. As far as I understand ci.swift.org does not seem to run LLVM testing by default, so even if the signals are green, that might still have LLVM testing failures. In our test infra we actually run those tests and find many of these broken commits from upstream.

I believe this is not correct. stable/20230725 was branched from upstream LLVM at 2023-07-25. If the commits in this PR were also placed upstream prior to that date, then they are also in stable/20230725.

Thanks, I get it, great. This change landed in August. So I should remove memory(*) in stable/20221013. Then cherry pick these commits in stable/20230725.

Again, if I understand correctly, stable/20221013 will never be merged into stable/20230725 (imagine them similar to LLVM release branches). I don't know what this changes are for, or why they are needed, but if you want them in future Apple releases, I think they need to be in both branches.

Since https://reviews.llvm.org/D135780 (what I think is a predependency of these changes) landed around November 2022, it is not in the 20221013 and it is probably in 20230725. https://reviews.llvm.org/D135780 (the one pointed in the summary) is probably in none of the branches, since it was closed in August 7th.

If you want the modifications here in the new stable branch, you probably need to also cherry-pick them there. For these older stable, you can try modifying the test to pass, or try to cherry-pick the predependency, but it looks like a big change, so it might need even more changes. I don't know if the maintainers will have better ideas.

@adrian-prantl
Copy link
Member

@DianQK The best path forward is to

  • open a new PR against stable/20221013 that backports the test such that check-llvm comes out clean.
  • there should be no need to cherry-pick this new PR anywhere else
  • double-check that the commits in this PR have been cherry-picked to stable/20230725.

@DianQK
Copy link
Author

DianQK commented Sep 12, 2023

Since https://reviews.llvm.org/D135780 (what I think is a predependency of these changes) landed around November 2022, it is not in the 20221013 and it is probably in 20230725. https://reviews.llvm.org/D135780 (the one pointed in the summary) is probably in none of the branches, since it was closed in August 7th.

@drodriguez
I rechecked this test case. memory(*) doesn't make sense. I can delete it.

The best path forward is

  • open a new PR against stable/20221013 that backports the test such that check-llvm comes out clean.
  • there should be no need to cherry-pick this new PR anywhere else
  • double-check that the commits in this PR have been cherry-picked to stable/20230725.

@adrian-prantl
Yeah, I'll do that. Thanks.

@DianQK DianQK mentioned this pull request Sep 12, 2023
Merged
DianQK added a commit to llvm/llvm-project that referenced this pull request Sep 14, 2023
@DianQK
[Coroutine][DebugInfo] Remove the memory attributes on coro-async-dec…
19b664d 
…laration.ll (NFC) (#66088)

According to @drodriguez's reminder in
apple#7168 (comment),
`memory` breaks the backport to the apple branch.

And this is irrelevant to that test. Delete to get better a test case.
kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Sep 14, 2023
@DianQK @kstoimenov
[Coroutine][DebugInfo] Remove the memory attributes on coro-async-dec…
52b3bff 
…laration.ll (NFC) (llvm#66088)

According to @drodriguez's reminder in
apple#7168 (comment),
`memory` breaks the backport to the apple branch.

And this is irrelevant to that test. Delete to get better a test case.
This was referenced Sep 17, 2023
Closed
Merged
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
@DianQK @ZijunZhaoCCK
[Coroutine][DebugInfo] Remove the memory attributes on coro-async-dec…
2924bae 
…laration.ll (NFC) (llvm#66088)

According to @drodriguez's reminder in
apple#7168 (comment),
`memory` breaks the backport to the apple branch.

And this is irrelevant to that test. Delete to get better a test case.
felipepiovezan pushed a commit that referenced this pull request Sep 22, 2023
@DianQK
[Coroutine][DebugInfo] Remove the memory attributes on coro-async-dec…
51ee87d 
…laration.ll (NFC) (llvm#66088)

According to @drodriguez's reminder in
#7168 (comment),
`memory` breaks the backport to the apple branch.

And this is irrelevant to that test. Delete to get better a test case.

(cherry picked from commit 19b664d)
2lambda123 pushed a commit to 2lambda123/apple-llvm-project that referenced this pull request May 20, 2024
@DianQK
[Coroutine][DebugInfo] Remove the memory attributes on coro-async-dec…
ae31a1e 
…laration.ll (NFC) (#66088)

According to @drodriguez's reminder in
apple/llvm-project#7168 (comment),
`memory` breaks the backport to the apple branch.

And this is irrelevant to that test. Delete to get better a test case.

(cherry picked from commit 19b664d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian-prantl adrian-prantl adrian-prantl approved these changes

@felipepiovezan felipepiovezan felipepiovezan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DianQK @adrian-prantl @felipepiovezan @drodriguez