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

MC/DC LLVM 19 intrinsics changes #126672

Open
krasimirgg opened this issue Jun 19, 2024 · 8 comments
Open

MC/DC LLVM 19 intrinsics changes #126672

krasimirgg opened this issue Jun 19, 2024 · 8 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@krasimirgg
Copy link
Contributor

In LLVM 19 some details about MC/DC changed, removing and updating some intrinsics:

I tried naively to remove the reference to the removed intrinsic #126582, but that's incomplete. Will need more work to adapt to this it seems:

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 19, 2024
@krasimirgg
Copy link
Contributor Author

@ZhuUx

@ZhuUx
Copy link
Contributor

ZhuUx commented Jun 19, 2024

As far as I know, works on mcdc need to be done for adapting to llvm 19 are:

  1. Eliminate the removed intrinsic function as you have done.
  2. Change the way to update condition bits as the new mechanism.
  3. Let ConditionId start from 0 rather than 1.

These changes are definite since they have been merged into llvm now, while still there are other changes may be introduced.

Given that 2 actually changes several arguments we passed to llvm and causes severe incompatibility, I'd wish to stop supporting mcdc on llvm-18 once if we switched to 19.

Edit. Other works have been done, but 2 introduces more break changes than I thought I'd need more time to adapt to it.

@Zalathar
Copy link
Contributor

In the short-term, we can just adjust the tests to not run on LLVM 19.

After the in-tree LLVM has been updated to LLVM 19, then we can remove support for MC/DC under LLVM 18, and add support for MC/DC under LLVM 19.

(Unless someone wants to do the extra work of supporting MC/DC with LLVM 19 before the official upgrade.)

@veera-sivarajan
Copy link
Contributor

@rustbot label -needs-triage +A-LLVM +T-compiler +C-enhancement

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 19, 2024
@workingjubilee
Copy link
Member

After the in-tree LLVM has been updated to LLVM 19, then we can remove support for MC/DC under LLVM 18, and add support for MC/DC under LLVM 19.

This sounds acceptable if we defer stabilizing MC/DC until our minimum LLVM is 19+

@ZhuUx
Copy link
Contributor

ZhuUx commented Jun 20, 2024

Filed #126733 to fix it and prepare for llvm 19.

@nikic
Copy link
Contributor

nikic commented Jul 24, 2024

If all goes well, I expect we'll try to land the LLVM 19 update next week, with broken MC/DC support if it's not ready in time.

@ZhuUx
Copy link
Contributor

ZhuUx commented Jul 25, 2024

The work has been done at #126733 . But it's blocked by #126677 , which has not been reviewed by the assignee yet.
No longer blocked, it's under review now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants