Skip to content

loongarch: Add basic support for LoongArch32 #1837

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heiher
Copy link
Contributor

@heiher heiher commented Jun 28, 2025

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Comment on lines +238 to +244
/// Calculate the approximate double-precision result of dividing 1.0 by the square root
#[inline]
#[target_feature(enable = "frecipe")]
#[unstable(feature = "stdarch_loongarch", issue = "117427")]
pub unsafe fn frsqrte_d(a: f64) -> f64 {
__frsqrte_d(a)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like llvm will just optimize this (no target feature is even needed): https://godbolt.org/z/5s6hvKE51.

Overall we prefer to have fewer (manual) extern "unadjusted¨ items. Also you should probably add some tests that these instructions actually produce the instructions you expect, like so

#[cfg_attr(all(test, target_feature = "frecipe"), assert_instr(frsqrt.d))]

assuming that frecipe is enabled on CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like llvm will just optimize this (no target feature is even needed): https://godbolt.org/z/5s6hvKE51.

They're different -- frsqrte_d_manual generates frsqrt.d, not frsqrt**e**.d.

Overall we prefer to have fewer (manual) extern "unadjusted¨ items. Also you should probably add some tests that these instructions actually produce the instructions you expect, like so

#[cfg_attr(all(test, target_feature = "frecipe"), assert_instr(frsqrt.d))]

assuming that frecipe is enabled on CI.

Sure, I'll add more test cases. Perhaps in a separate PR to keep things focused. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, my bad. so the "e" is for "estimate" apparently. Maybe use that word? (most functions on floats approximate the answer, but apparently this intrinsic deliberately uses less precision to achieve higher speed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. 😄

@sayantn
Copy link
Contributor

sayantn commented Jul 7, 2025

We will also have to add a LA32 CI run. It doesn't seem like qemu supports LA32 yet (can we run it on qemu-loongarch64 work?), and rustc has only bare-metal targets with this arch (we can probably work around that by having a loongarch32-unknown-linux-gnu.json target spec file). Also it doesn't seem like the Ubuntu repo has a loongarch32-gcc package (is it available from somewhere else, like aarch64_be-gcc?)

@tgross35
Copy link
Contributor

tgross35 commented Jul 7, 2025

Usually we don't run CI for T3 targets, e.g. Mips is skipped. They need to move to T2 to get to blocking CI.

riscv64gc is technically T3, but that one is probably fine since it's a superset of the T2 riscv32 targets.

#[link_name = "llvm.loongarch.cacop.w"]
fn __cacop(a: i32, b: i32, c: i32);
#[link_name = "llvm.loongarch.csrrd.w"]
fn __csrrd(a: i32) -> i32;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to have these in the common code returning isize? It is technically a breaking change but this is still an unstable feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments. I don't quite understand why this is considered a breaking change. Previously, LoongArch64 exported csrrd(i32) -> i64, and it sill does. Now, LoongArch32 is introducing csrrd(i32) -> i32, which doesn't change the existing behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants