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.

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

[DO NOT MERGE] llvm-wrapper: adapt for LLVM 19 API change #126582

Closed
wants to merge 0 commits into from

Conversation

krasimirgg
Copy link
Contributor

No functional changes intended.

The instrprof_mcdc_condbitmap_update intrinsic was dropped recently:

@rustbot label: +llvm-main

@rustbot
Copy link
Collaborator

rustbot commented Jun 17, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) labels Jun 17, 2024
@rust-log-analyzer

This comment has been minimized.

@krasimirgg krasimirgg marked this pull request as ready for review June 17, 2024 12:43
@krasimirgg
Copy link
Contributor Author

r? @durin42

@rustbot rustbot assigned durin42 and unassigned cuviper Jun 17, 2024
@nikic
Copy link
Contributor

nikic commented Jun 17, 2024

Does LLVMRustGetInstrProfMCDCCondBitmapIntrinsic not get called on LLVM 19?

@durin42
Copy link
Contributor

durin42 commented Jun 17, 2024

The test suite passes with this applied, so if it's a problem on LLVM 19 there's some missing test coverage I guess?

I couldn't easily prove to myself this function won't get called on LLVM 19 tho...

@nikic
Copy link
Contributor

nikic commented Jun 18, 2024

cc @ZhuUx @Zalathar who worked on MC/DC in Rust.

@durin42 Is that based on a full local test run, or the buildkite integration? Because I don't think that one runs coverage tests.

@durin42
Copy link
Contributor

durin42 commented Jun 18, 2024 via email

@ZhuUx
Copy link
Contributor

ZhuUx commented Jun 18, 2024

The test suite passes with this applied, so if it's a problem on LLVM 19 there's some missing test coverage I guess?

I couldn't easily prove to myself this function won't get called on LLVM 19 tho...

Which llvm commit the tests based on? I failed to compile rustc with the latest llvm 19. It breaks when compiling compiler-builtins.

We do should fix something if we're going to upgrade llvm to 19, which not only includes this api change. But I'm confused about why this patch does not panic when testing tests/coverage/mcdc_if.rs.

@ZhuUx
Copy link
Contributor

ZhuUx commented Jun 18, 2024

Considering there are something to do on mcdc with llvm-19, if it's urgent for us to upgrade llvm-19, we can temporarily disable mcdc functions:

  1. Add something like max-llvm-version to tests/coverage/mcdc_if.rs, tests/coverage/mcdc_nested_if.rs and all tests under tests/coverage/mcdc (All tests are going to move into test/coverage/mcdc but that pr is under progress)
  2. Wrap all related functions in RustWrapper.cpp with LLVM_VERSION_GE(18, 0) && LLVM_VERSION_LT(19, 0).

Otherwise if there is still some time to wait I would draft a pr to fix them tomorrow.

@nikic
Copy link
Contributor

nikic commented Jun 18, 2024

@ZhuUx It's not urgent.

@krasimirgg
Copy link
Contributor Author

OK, so this is incomplete. Filed #126672 to track the proper fix.

@krasimirgg krasimirgg changed the title llvm-wrapper: adapt for LLVM 19 API change [DO NOT MERGE] llvm-wrapper: adapt for LLVM 19 API change Jun 19, 2024
@bors
Copy link
Contributor

bors commented Jun 27, 2024

☔ The latest upstream changes (presumably #123918) made this pull request unmergeable. Please resolve the merge conflicts.

@zmodem
Copy link
Contributor

zmodem commented Jul 1, 2024

@krasimirgg any chance of getting a rebased version of this?

@krasimirgg
Copy link
Contributor Author

krasimirgg commented Jul 1, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm-main Marks PRs that are making Rust work with LLVM main (this label is consumed by CI tooling) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants