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

Add linux-loongarch64 CI leg #111086

Merged
merged 12 commits into from
Jan 8, 2025

Conversation

am11
Copy link
Member

@am11 am11 commented Jan 4, 2025

No description provided.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 4, 2025
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@am11 am11 requested review from jkoritzinsky and janvorli January 6, 2025 11:15
@@ -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>
Copy link
Member Author

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.

@jkotas
Copy link
Member

jkotas commented Jan 6, 2025

linux-loongarch64 Release AllSubsets_Mono

This should be adding CoreCLR leg only.

@am11
Copy link
Member Author

am11 commented Jan 6, 2025

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.

@jkotas
Copy link
Member

jkotas commented Jan 6, 2025

My point is that there should not be "Build linux-loongarch64 Release AllSubsets_Mono" leg in the first place.

@am11
Copy link
Member Author

am11 commented Jan 6, 2025

@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.

@jkotas
Copy link
Member

jkotas commented Jan 6, 2025

If nothing else, it is misleading. Can we have a single loongarch leg that builds coreclr and corehost only (without mono)?

@jkotas
Copy link
Member

jkotas commented Jan 6, 2025

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.

@am11
Copy link
Member Author

am11 commented Jan 6, 2025

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.

@jkotas
Copy link
Member

jkotas commented Jan 6, 2025

Removing riscv mono support would be a step backward

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.

@am11
Copy link
Member Author

am11 commented Jan 6, 2025

FreeBSD is using such a template which excludes mono. I will switch to that one. It will help with #105004.

@am11
Copy link
Member Author

am11 commented Jan 7, 2025

I have excluded the packs subset from community platform which requires the live apphost. This PR is independent of the other two now.

@am11
Copy link
Member Author

am11 commented Jan 7, 2025

@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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@jkotas jkotas left a 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?

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes LGTM

@jkotas
Copy link
Member

jkotas commented Jan 8, 2025

/ba-g Azure DevOps can't recover from restarts.

@jkotas jkotas merged commit 043e60b into dotnet:main Jan 8, 2025
142 of 148 checks passed
@am11 am11 deleted the feature/ci/loongarch64-pr-validation branch January 8, 2025 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-loongarch64 area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants