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.

Sign up for GitHub

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

Jump to bottom

[LoongArch64] Add nativeaot support on LoongArch64. #103889

Merged
merged 11 commits into from
Jul 6, 2024
Merged

[LoongArch64] Add nativeaot support on LoongArch64. #103889

merged 11 commits into from
Jul 6, 2024

Conversation

sunlijun-610
Copy link
Contributor

This PR adds nativeaot support on LoongArch64.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 24, 2024
@sunlijun-610
Copy link
Contributor Author

Hi, @MichalStrehovsky @shushanhf
Could you please review this PR? Thanks!

This was referenced Jun 24, 2024
Open
Closed
Open
Open
Closed
filipnavara
filipnavara reviewed Jun 24, 2024
View reviewed changes
src/coreclr/nativeaot/Runtime/PalRedhawkCommon.h Outdated Show resolved Hide resolved
src/coreclr/nativeaot/Runtime/loongarch64/PInvoke.S Outdated Show resolved Hide resolved
src/coreclr/nativeaot/Runtime/loongarch64/PInvoke.S Outdated Show resolved Hide resolved
src/coreclr/nativeaot/Runtime/unix/UnwindHelpers.cpp Outdated Show resolved Hide resolved
...r.Compiler/Compiler/DependencyAnalysis/Target_LoongArch64/LoongArch64ReadyToRunHelperNode.cs Outdated Show resolved Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/JitHelper.cs Show resolved Hide resolved
...reclr/tools/aot/ILCompiler.Compiler/Compiler/ObjectWriter/CodeView/CodeViewSymbolsBuilder.cs Outdated Show resolved Hide resolved
src/native/external/llvm-libunwind/src/CompactUnwinder.hpp Outdated Show resolved Hide resolved
src/native/external/llvm-libunwind/src/Registers.hpp Outdated Show resolved Hide resolved
@sunlijun-610
Copy link
Contributor Author

Excuse me for giving you so much trouble. Cause this is my first time submitting code, I made many low-level mistakes. Thanks very much for your reviews and guidance on me!
I’ve submitted the second patch correcting the mistakes.
Thanks!

@filipnavara
Copy link
Member

Excuse me for giving you so much trouble. Cause this is my first time submitting code, I made many low-level mistakes. Thanks very much for your reviews and guidance on me!

No trouble at all. The purpose of the review process is to find those mistakes and it seems to work reasonably well here.

Side-note: I skimmed over most of the PR changes and looked for things that bit me during the linux-arm / win-x86 ports. The object writer part looks alright save for one comment.

@build-analysis build-analysis bot mentioned this pull request Jun 25, 2024
Open
3 tasks
@jkotas
Copy link
Member

jkotas commented Jun 26, 2024

No trouble at all.

+1

This change is a lot of lines that touching number of different areas. It tends to be better to split changes like this into multiple smaller PRs. For example:

  • Runtime assembly code (*.S) file
  • Changes in ObjectWriter component
  • Everything else

If you prefer to continue with the PR as is, it is certainly fine - but it is likely to take longer to get it in compared if it is broken into multiple smaller PRs.

@sunlijun-610
Copy link
Contributor Author

sunlijun-610 commented Jun 26, 2024 •
edited
Loading

This change is a lot of lines that touching number of different areas. It tends to be better to split changes like this into multiple smaller PRs.

OK, got it. I'll split this PR into multiple smaller PRs as soon as possible. Thanks!

@sunlijun-610
Copy link
Contributor Author

sunlijun-610 commented Jun 27, 2024 •
edited
Loading

After splitting:
Part-2 has been submitted to new PR: #104084
Part-3 has been submitted to new PR: #104087
Part-4 has been submitted to this PR, covering the previous, and it's the last PR.

This was referenced Jun 27, 2024
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Jul 2, 2024
Closed
shushanhf
shushanhf approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

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

shushanhf
shushanhf reviewed Jul 2, 2024
View reviewed changes
Copy link
Contributor

@shushanhf shushanhf left a comment

Choose a reason for hiding this comment

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

sorry for mistake click.
@sunlijun-610 please test these PRs based on the new PRs.
Thanks

@build-analysis build-analysis bot mentioned this pull request Jul 3, 2024
Closed
@sunlijun-610
Copy link
Contributor Author

sunlijun-610 commented Jul 4, 2024 •
edited
Loading

@sunlijun-610 please test these PRs based on the new PRs.

I've tested based on the new PRs and the latest commit, and no new failure cases added.
Thanks!

jkotas
jkotas approved these changes Jul 4, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM modulo one last nit

@build-analysis build-analysis bot mentioned this pull request Jul 5, 2024
Open
@jkotas
Copy link
Member

jkotas commented Jul 5, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Jul 6, 2024

Filled #104500 on windows-arm64 Release NativeAOT_Libs failure

@jkotas jkotas merged commit aee78d3 into dotnet:main Jul 6, 2024
163 of 169 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shushanhf shushanhf shushanhf approved these changes

@huoyaoyuan huoyaoyuan huoyaoyuan left review comments

@am11 am11 am11 left review comments

@filipnavara filipnavara filipnavara left review comments

@jkotas jkotas jkotas approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees
No one assigned
Labels
arch-loongarch64 area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sunlijun-610 @filipnavara @jkotas @am11 @huoyaoyuan @shushanhf