-
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
Add linux-loongarch64 CI leg #111086
Add linux-loongarch64 CI leg #111086
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
@@ -10,7 +10,7 @@ | |||
<!-- Force System.Private.CoreLib.dll into a special IL output directory --> | |||
<OutputPath>$(RuntimeBinDir)IL/</OutputPath> | |||
<Configurations>Debug;Release;Checked</Configurations> | |||
<Platforms>x64;x86;arm;armv6;arm64;riscv64;s390x;wasm;ppc64le</Platforms> | |||
<Platforms>x64;x86;arm;arm64</Platforms> |
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.
Aligned with coreclr. Platforms is relevant in VS context.
This should be adding CoreCLR leg only. |
The failure is not from mono, it's from corehost tests as I mentioned above. I'm waiting for the new image build dotnet/dotnet-buildtools-prereqs-docker#1311. The caching in docker layer is not ideal so we have to go back and forth each time there is a new platform. |
My point is that there should not be "Build linux-loongarch64 Release AllSubsets_Mono" leg in the first place. |
@jkotas the name of this leg is a bit misleading but it is basically validating the all native code is compiling including libs and corehost. We have the same setup for riscv. There is no problem in building native mono on loongarch as we can see in the logs. |
If nothing else, it is misleading. Can we have a single loongarch leg that builds coreclr and corehost only (without mono)? |
riscv64 should be on the same plan. I do not think we want to continue to invest into riscv mono port, for the same reason we have rejected loongarch64 mono port. |
Removing riscv mono support would be a step backward IMO, but nonetheless, it should be done in main as a separate PR. This PR is adding loongarch next to riscv to keep it in sync. Future infra refactoring can be applied to both at the same time. |
It has been becoming more apparent to us (core .NET team) that we do not have capacity to maintain two completely different runtimes that are equivalent. If you would like to remove riscv mono support in a separate PR, that's fine; but this PR should not be adding mono loongarch to the CI. |
FreeBSD is using such a template which excludes mono. I will switch to that one. It will help with #105004. |
I have excluded the packs subset from community platform which requires the live apphost. This PR is independent of the other two now. |
@jkotas, linux-loongarch/riscv64 and freebsd-x64 are on the same CI plan without mono. Please take a look. |
FORCEINLINE uint8_t PalInterlockedCompareExchange128(_Inout_ int64_t volatile *pDst, int64_t iValueHigh, int64_t iValueLow, int64_t *pComparandAndResult) | ||
{ | ||
__int128_t iComparand = ((__int128_t)pComparandAndResult[1] << 64) + (uint64_t)pComparandAndResult[0]; | ||
// NOTE: for LoongArch64, it supports 128bits atomic from 3A6000-CPU which is ISA1.1's version. | ||
// The LA64's compiler will translate the `__sync_val_compare_and_swap` into calling the libatomic's library interface to emulate |
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.
Does the build target ISA1.0 or ISA1.1 by default? If it targets ISA1.0 by default, this implementation is broken. This should be TODO instead of NOTE, ideally with a link to an issue that tracks fixing 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.
Changed to TODO linking #109276. This is @shushanhf's comment. CI cross build is not linking libatomic, so it should be ISA1.1.
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.
LGTM. Thanks!
@jkoritzinsky Could you please review the CI setup?
Co-authored-by: Jan Kotas <[email protected]>
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.
Build changes LGTM
/ba-g Azure DevOps can't recover from restarts. |
No description provided.