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

LoongArch is a NEW ISA which is different from MIPS #113564

Merged
merged 5 commits into from
Mar 17, 2025
Merged

LoongArch is a NEW ISA which is different from MIPS #113564

merged 5 commits into from
Mar 17, 2025

Conversation

4Darmygeometry
Copy link
Contributor

reference:https://docs.kernel.org/arch/loongarch/introduction.html
Introduction to LoongArch

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 15, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 15, 2025
huoyaoyuan
huoyaoyuan reviewed Mar 15, 2025
View reviewed changes
src/coreclr/jit/codegenlinear.cpp Outdated
Comment on lines 2487 to 2488
// For LoongArch64's ISA which is a RISC ISA which is different from any other existing ones,
// even the instructions of 32bits operation need the upper 32bits be sign-extended to 64 bits.
Copy link
Member

Choose a reason for hiding this comment

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

The comment should be saying "LoongArch64 is using the same mechanism with MIPS64 in this scenario"

Copy link
Member

Choose a reason for hiding this comment

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

@shushanhf Can you help to clarify the meaning of this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be change to:
LoongArch64 is using the similar mechanism with MIPS64 in this scenario

Copy link

Choose a reason for hiding this comment

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

Can be change to: LoongArch64 is using the similar mechanism with MIPS64 in this scenario

The problem is LoongArch64 does NOT have this mechanism (except for division instructions). So this would be still incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can be change to: LoongArch64 is using the similar mechanism with MIPS64 in this scenario

Yes, here LA64 has to keep similar mechanism with MIPS64.

Copy link
Contributor

@shushanhf shushanhf Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can be change to: LoongArch64 is using the similar mechanism with MIPS64 in this scenario

The problem is LoongArch64 does NOT have this mechanism (except for division instructions). So this would be still incorrect.

Although the LA64 is not same with MIPS64 compeletely, liking you said there are still some instructions needing the upper 32bits be sign-extended to 64 bits, we have to keep the similar mechanism for LA64.
If you delete these code it will introduce some errors when you running the test cases.

@4Darmygeometry
Copy link
Contributor Author

#113565

xry111
xry111 suggested changes Mar 15, 2025
View reviewed changes
Copy link

@xry111 xry111 left a comment

Choose a reason for hiding this comment

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

Marking "changes requested" as the modified comment is still incorrect.

This was referenced Mar 16, 2025
Open
Open
@jkotas
Copy link
Member

jkotas commented Mar 17, 2025

cc @dotnet/samsung

@jkotas jkotas added the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 17, 2025
@jkotas jkotas requested a review from xry111 March 17, 2025 13:36
@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Mar 17, 2025
jkotas and others added 2 commits March 17, 2025 07:39
@jkotas @am11
Update src/coreclr/jit/codegenlinear.cpp
1ad05ee 
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@4Darmygeometry
Merge branch 'dotnet:main' into main
1558fd4
@rashworld-max rashworld-max mentioned this pull request Mar 17, 2025
Open
@jkotas jkotas merged commit ec77954 into dotnet:main Mar 17, 2025
111 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@am11 am11 am11 left review comments

@huoyaoyuan huoyaoyuan huoyaoyuan left review comments

@shushanhf shushanhf shushanhf left review comments

@tomeksowi tomeksowi tomeksowi approved these changes

@jkotas jkotas jkotas approved these changes

@rashworld-max rashworld-max rashworld-max approved these changes

@xry111 xry111 Awaiting requested review from xry111

Assignees
No one assigned
Labels
arch-loongarch64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI 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.
8 participants
@4Darmygeometry @jkotas @tomeksowi @am11 @huoyaoyuan @xry111 @shushanhf @rashworld-max