-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Bump LLVM to 19.x branch #103585
Bump LLVM to 19.x branch #103585
Conversation
@lambdageek, would you have time to look at porting mono to llvm 19.x? It uses our packages from general testing channel built with unreleased llvm 19.x, same commit as emscripten 3.1.56 uses plus our changes re-based on top of it, https://github.com/dotnet/llvm-project/tree/dotnet/main-19.x /cc @lewing |
@radekdoulik Not sure. /cc @steveisok Looks like the build failure is due to which removed the C bindings to the legacy pass manager. I don't know a ton about this part of Mono. |
Tagging subscribers to this area: @lambdageek, @steveisok |
@radekdoulik can you update this: https://github.com/dotnet/llvm-project/blob/dotnet/main-19.x/llvm/CMakeLists.txt#L22 I'm trying to do a local build of this PR with the new pass manager commented out, so that I can check what else is broken |
@radekdoulik here's some commits to at least get the thing building this isn't a good long term solution, but it will at least get us to the point where we can see what else needs to be fixed up |
oh all this LLVM pass manager stuff is for the LLVM JIT - we should just drop support for it. |
Made a half-hearted attempt at supporting the new passbuilder for the LLVM JIT. I'm pretty sure it doesn't work (I don't know how to get a https://github.com/lambdageek/runtime/commits/radek-pr-bump-llvm-19-plus-passbuilder/ Even if we don't do the new passbuilder, we do need to update this stuff for LLVM-19. The runtime linked without updates for me if I don't touch the new passbuilder, but I'm sure something else would break. runtime/src/mono/CMakeLists.txt Lines 611 to 635 in 0498939
|
doing: $ make -c src/mono/sample/HelloWorld clean publish run AOT=true USE_LLVM=true crashes in
Do we have the mono calling convention patches on the 19.x branch? |
They dont' have a C API for it anymore. We should use the new PassBuilder API instead. But temporarily just disable optimizations entirely
This reverts commit efca5a2.
Remove coherency between llvm and emscripten to be able to update llvm 19.x packages with darc
/azp run runtime-ioslike |
Azure Pipelines successfully started running 1 pipeline(s). |
Previous build was green enough, we plan to merge. |
/ba-g fast merging to unblock flow from other repos |
@@ -17,6 +17,9 @@ | |||
<!-- Active issue: https://github.com/dotnet/runtime/issues/97809 --> | |||
<ShouldILStrip>false</ShouldILStrip> | |||
</PropertyGroup> | |||
<PropertyGroup Condition="'$(TargetOS)' == 'browser'"> | |||
<XunitShowProgress>true</XunitShowProgress> | |||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep it in, just in case the issue reappear, to have more information about which test is causing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We recently removed the progress printing since it was thought to cause timeouts: #103621
I'm fine if we keep it for a couple days but we should ultimately remove it
</ExcludeList> | ||
<ExcludeList Include = "$(XunitTestBinBase)/JIT/Methodical/Methodical_ro/**"> | ||
<Issue>Crashes during LLVM AOT compilation.</Issue> | ||
</ExcludeList> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to file an issue for these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you remember where did it fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, it's failing on fullAOT-mini lane as well #105395. It's possible that it's related to another change in the range since there were a few of [mono] changes during the period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why is it still failing even after disabling the test for all Mono... https://dev.azure.com/dnceng-public/public/_build/results?buildId=753237&view=logs&j=9c845561-93d9-5f12-0979-955ea2f35497&t=644e769d-37f3-54b4-c202-c63c42bf5d0c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this is an incorrect way to disable tests with aot. I used to get failures with these tests on llvm fix PR #105867. Testing locally there were failures for missing aot image because the tests where not being aot compiled but we still tried to run them. Together with the other llvm fixes, enabling these back fixed the failures.
Is it possible we are seeing a 0.2MB size increase on iOS due to the LLVM 19 bump ? This is the range fdef8db...d46fdc1 and I don't see anything else suspicious. |
Definitely possible yeah |
Do we use |
changing the optimization flags can have other unintended consequences. tbh I wouldn't worry about it for a 200k diff on iOS. |
I agree - I think we can live with it |
I wasn't only thinking about this regression but the overall size increase we were experiencing during .NET 9 cycle (#105701). However, it's just an idea, improvements to trimming and such are likely a better approach. |
No description provided.