Skip to content

[LoongArch64] coreclr-vm directory #62885

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
Merged
merged 49 commits into from
Apr 21, 2022
Merged

[LoongArch64] coreclr-vm directory #62885

merged 49 commits into from
Apr 21, 2022

Conversation

@shushanhf
Copy link
Contributor

@shushanhf shushanhf commented Dec 16, 2021

Part6-2: Add the coreclr-vm directory for LoongArch64.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-VM-coreclr labels Dec 16, 2021
@dnfadmin
Copy link

dnfadmin commented Dec 16, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@shushanhf shushanhf mentioned this pull request Dec 16, 2021
Merged
@jkotas jkotas changed the title Part6-2: Add the coreclr-vm directory for LoongArch64. [LoongArch64] coreclr-vm directory Dec 16, 2021
jkotas
jkotas reviewed Dec 16, 2021
View reviewed changes
kant2002
kant2002 reviewed Dec 17, 2021
View reviewed changes
Copy link
Contributor

@kant2002 kant2002 left a comment

Choose a reason for hiding this comment

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

Left some comments.

shushanhf
shushanhf commented Dec 18, 2021
View reviewed changes
@kant2002 kant2002 mentioned this pull request Dec 18, 2021
Merged
jkotas
jkotas reviewed Dec 18, 2021
View reviewed changes
jkotas
jkotas reviewed Dec 18, 2021
View reviewed changes
src/coreclr/vm/argdestination.h Outdated
void CopyStructToRegisters(void *src, int fieldBytes)
{
_ASSERTE(IsStructPassedInRegs());
_ASSERTE(m_argLocDescForStructInRegs->m_cFloatReg == 1);
Copy link
Member

@jkotas jkotas Dec 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Where is the case of struct passed in two floating registers handled?

Copy link
Contributor Author

@shushanhf shushanhf Dec 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Where is the case of struct passed in two floating registers handled?

A struct containing just one floating-point real is passed as though it were a standalone floating-point real.
It's related with the ABI, similar with the PR #62893 ,
#62893 (comment)

This case is also the same with the RISC-V: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc

kant2002
kant2002 reviewed Dec 18, 2021
View reviewed changes
Copy link
Contributor

@kant2002 kant2002 left a comment

Choose a reason for hiding this comment

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

Some comments about compilation issues.

qiaopengcheng and others added 3 commits December 20, 2021 10:32
[LoongArch64] moved vm/jitinterface.cpp from #62893.
7d03307 
moved inc/corinfo.h from #6288.
@shushanhf
[LoongArch64] run the file `src/coreclr/tools/Common/JitInterface/Thu…
f6ec562 
…nkGenerator/gen.sh`

and merge the patch from #62893.
@shushanhf
[LoongArch64] revert vm/ceeload.h.
277de6f 
amend the `ToolBox/superpmi/superpmi-shared/agnostic.h`
@shushanhf shushanhf mentioned this pull request Dec 20, 2021
Merged
jkotas
jkotas reviewed Mar 3, 2022
View reviewed changes
jkotas
jkotas reviewed Mar 7, 2022
View reviewed changes
jkotas
jkotas reviewed Mar 7, 2022
View reviewed changes
jkotas
jkotas reviewed Mar 7, 2022
View reviewed changes
@shushanhf
[LoongArch64] Amend the LoongArch64's ABI
6566991 
after merged `MethodTable::GetLoongArch64PassStructInRegisterFlags()`
and `CEEInfo::getLoongArch64PassStructInRegisterFlags()`
@BruceForstall
Copy link
Contributor

BruceForstall commented Mar 21, 2022

@shushanhf Now that #65738 has merged, this needs to be updated.

@shushanhf
Copy link
Contributor Author

shushanhf commented Mar 22, 2022

@shushanhf Now that #65738 has merged, this needs to be updated.

OK, Thanks

@BruceForstall
Copy link
Contributor

BruceForstall commented Mar 22, 2022

@jkotas @janvorli Time for another (final?) code review here?

janvorli
janvorli reviewed Apr 4, 2022
View reviewed changes
@shushanhf
[LoongArch64] Fix the assert error
d6c7080 
when running hello-world within debug-mode after refacting.
@shushanhf
Copy link
Contributor Author

shushanhf commented Apr 8, 2022 •
edited
Loading

Now I had tested the hello-world passed both the release and debug mode based on the latest main-LoongArch64.

@BruceForstall
Copy link
Contributor

BruceForstall commented Apr 8, 2022

@mangod9 @janvorli @jkotas Is there anything left here to review? (fyi, I just merged the LoongArch64 JIT PR)

@shushanhf shushanhf requested a review from janvorli April 13, 2022 02:46
@shushanhf
Copy link
Contributor Author

shushanhf commented Apr 13, 2022

@mangod9 @janvorli @jkotas
Could you please give me some advices further?

@shushanhf
Copy link
Contributor Author

shushanhf commented Apr 20, 2022 •
edited
Loading

Hi, @mangod9 @janvorli @jkotas

What should I do for this PR next？
Could you please give me some advices ?

@mangod9
Copy link
Member

mangod9 commented Apr 20, 2022

Hi @shushanhf, sorry for delayed response. We plan to review this again this week and will merge with a green CI. Thanks

janvorli
janvorli approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mangod9
Copy link
Member

mangod9 commented Apr 21, 2022

Some JIT HW Intrinsics tests are failing -- I will submit a rerun.
cc @BruceForstall in case these have been seen before: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-62885-merge-13bbab73369a44af9a/JIT.HardwareIntrinsics/1/console.c2df8b25.log?sv=2019-07-07&se=2022-04-28T05%3A53%3A25Z&sr=c&sp=rl&sig=B24gI9K1nVgOTBqIiWnGKoSByMPjTbvq0VZSpMHTCbc%3D

@mangod9
Copy link
Member

mangod9 commented Apr 21, 2022

Looks to have passed on rerun. So will merge it now.

@mangod9 mangod9 merged commit febeba3 into dotnet:main Apr 21, 2022
@BruceForstall
Copy link
Contributor

BruceForstall commented Apr 21, 2022

@shushanhf Thanks for your patience and response to code review feedback. And congratulations on getting this merged!

@shushanhf shushanhf deleted the main_loongarch64_2 branch May 11, 2022 01:12
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@janvorli janvorli janvorli approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky

@jkotas jkotas Awaiting requested review from jkotas

@mangod9 mangod9 Awaiting requested review from mangod9

+2 more reviewers

@am11 am11 am11 left review comments

@kant2002 kant2002 kant2002 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-loongarch64 area-VM-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

8 participants

@shushanhf @dnfadmin @kant2002 @jkotas @mangod9 @BruceForstall @janvorli @am11