This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add LoongArch64 support
ClosedPublic

Authored by prcups on Aug 30 2022, 3:07 AM.

Details

Summary

LoongArch64 is a new risc architecture established by Loongson. There has been PC which contains 3A5000 using this architecture. Now gcc, glibc and binutils have added support for it, while linux mainline will fully support it after 6.1.
This patch adds support for LLVM OpenMP following D59880 for RISCV64.

Diff Detail

Event Timeline

prcups created this revision.Aug 30 2022, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 3:07 AM
prcups requested review of this revision.Aug 30 2022, 3:07 AM

Thanks.

This touches files like D59880 did except openmp/runtime/src/thirdparty/ittnotify/ittnotify_config.h and outdated openmp/www. For ittnotify I suppose we do not need to change it because D109333.

Some concerns:

  1. Have you tested existing testcases on a native LoongArch machine? If so, do you use clang or gcc?
  2. Do we need to add new tests?
openmp/runtime/CMakeLists.txt
189

nit: Maybe LoongArch64 is better.

openmp/runtime/src/z_Linux_asm.S
1762–1770

What does that mean?

1824

Nit: useless blank line

1833

Nit: generally speaking, comment sentence should ends with a dot('.').

Thanks.

This touches files like D59880 did except openmp/runtime/src/thirdparty/ittnotify/ittnotify_config.h and outdated openmp/www. For ittnotify I suppose we do not need to change it because D109333.

Some concerns:

  1. Have you tested existing testcases on a native LoongArch machine? If so, do you use clang or gcc?
  2. Do we need to add new tests?

For 1, I tried to use "make check", unfortunately, almost all test unit failed. Threads crash just before ending and it seems to be about stack. The crash happened at the end of process kmp_invoke_task_func near kmp_run_after_invoked_task. I 'm trying to find the possible cause.
For 2, because RISC-V doesn't have particular test, and I think common tests on loongarch are adequate to find errors, it is not neccessary to add new tests.

prcups updated this revision to Diff 457203.EditedSep 1 2022, 1:59 AM

Now I have fixed the problem(The cause is just about not saving return address before calling). Although that, it still failed 4 tests after I ran "make check-openmp". The failed tests are affinity/kmp-affinity.c, affinity/kmp-hw-subset.c, affinity/omp-places.c and teams/teams.c. I 'm not sure if it's necessary to care about and how to handle them. There are also 1 unexpextedly passed and 3 expectedly failed tests.

The comment in CMakeList.txt has been fixed. The 1762-1770 lines in z_Linux_asm.S are just rubbish code I carelessly pasted.

For the dot after comment, in z_Linux_asm.S, it is the same as riscv. Is it also necessary to modify them?

Now I have fixed the problem(The cause is just about not saving return address before calling). Although that, it still failed 4 tests after I ran "make check-openmp". The failed tests are affinity/kmp-affinity.c, affinity/kmp-hw-subset.c, affinity/omp-places.c and teams/teams.c. I 'm not sure if it's necessary to care about and how to handle them. There are also 1 unexpextedly passed and 3 expectedly failed tests.

The comment in CMakeList.txt has been fixed. The 1762-1770 lines in z_Linux_asm.S are just rubbish code I carelessly pasted.

For the dot after comment, in z_Linux_asm.S, it is the same as riscv. Is it also necessary to modify them?

Excellent! I haven’t saw this ra issue when I review the patch.

I think you’re using gcc to build the openmp since clang currently is unable to build such complex programs, right?

AFAIK, some tests can only be run by clang. check-openmp is only part of all tests. There are some other targets like check-libomptarget IIRC. So I think it’s better to mention your testing environment, methods and results in the patch summary.

And I’m not a regular reviewer of the openmp subproject. Let’s wait others’ inputs.

xen0n added a comment.Sep 1 2022, 9:17 PM

Thanks and welcome to the LLVM community! Some minor comments regarding LoongArch and general coding style.

openmp/README.rst
140

The list is alphabetically sorted, so please put this before "mic".

openmp/runtime/src/kmp_csupport.cpp
703 ↗(On Diff #457203)

Similarly place this before MIPS.

openmp/runtime/src/z_Linux_asm.S
1762–1770

I guess it means the return value is in a0 which is always 1.

1777

RISCV instruction words can be 2 bytes long due to RVC, but LoongArch insns are always 4 bytes long. So the p2align value probably should be 2.

1817–1818

LoongArch base ISA has fairly decent bitops, so we can simply bstrins.d $sp, $zero, 3, 0 to clear the lowest 4 bits. (Literally "insert $zero's content into $sp[3:0]".)

openmp/runtime/tools/lib/Platform.pm
103

Place before "mic".

prcups updated this revision to Diff 457504.EditedSep 1 2022, 10:45 PM

I have followed these instructions to modify it.

xry111 added inline comments.Sep 2 2022, 1:36 AM
openmp/runtime/src/z_Linux_asm.S
1802–1808

Let's be less tricky and reduce one instruction.

xry111 added inline comments.Sep 3 2022, 1:32 AM
openmp/runtime/src/z_Linux_asm.S
1806

Hmm, looks like something has gone wrong here when my change is applied... addi.d $t0, $t0, 1 is duplicated, and addi.d $t0, $a3, -6 is missing.

prcups updated this revision to Diff 457826.Sep 3 2022, 7:22 PM
SixWeining added inline comments.Sep 3 2022, 8:39 PM
openmp/runtime/src/z_Linux_asm.S
1805

Should place this line before line 1802?

prcups updated this revision to Diff 457827.Sep 3 2022, 8:43 PM
prcups marked 11 inline comments as done.Sep 4 2022, 4:04 AM
SixWeining accepted this revision.Sep 4 2022, 8:52 PM

This change LGTM regarding LoongArch.

a samll tip: The patch's summary can be edited in this web page in case you want to mention the testing result. :)

This revision is now accepted and ready to land.Sep 4 2022, 8:52 PM
prcups accepted this revision.Sep 5 2022, 3:37 AM

@MaskRay So could you help me commit these changes?

@MaskRay So could you help me commit these changes?

I'll do some verification.

MaskRay accepted this revision.Sep 16 2022, 7:55 PM

This works on a GCC compiler farm machine.

gcc -fopenmp a.c -g -o a -I ~/llvm/out/release/projects/openmp/runtime/src ~/llvm/out/release/lib/libomp.so -Wl,-rpath=$HOME/llvm/out/release/lib
./a  # good
MaskRay added inline comments.Sep 18 2022, 2:30 PM
openmp/runtime/src/kmp_csupport.cpp
703 ↗(On Diff #457203)

Revert this change after D130928

MaskRay retitled this revision from Add LoongArch64 Support For OpenMP to [OpenMP] Add LoongArch64 support.Sep 19 2022, 3:48 PM
MaskRay edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.

Now I have fixed the problem(The cause is just about not saving return address before calling). Although that, it still failed 4 tests after I ran "make check-openmp". The failed tests are affinity/kmp-affinity.c, affinity/kmp-hw-subset.c, affinity/omp-places.c and teams/teams.c. I 'm not sure if it's necessary to care about and how to handle them. There are also 1 unexpextedly passed and 3 expectedly failed tests.

Some of failed cases could be fixed by D139802.