This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][LoongArch] Add 64-bit LoongArch support
ClosedPublic

Authored by Ami-zhang on Oct 28 2022, 11:31 PM.

Details

Summary

Defines enums for the LoongArch registers.
Adds the register class implementation for LoongArch.
Adds save and restore context functionality.

This only supports 64 bits integer and float-point register
implementation.

Fix https://github.com/llvm/llvm-project/issues/55398

Depends on D136413

Diff Detail

Event Timeline

Ami-zhang created this revision.Oct 28 2022, 11:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 28 2022, 11:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Ami-zhang requested review of this revision.Oct 28 2022, 11:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2022, 11:31 PM
MaskRay added inline comments.Oct 28 2022, 11:49 PM
libunwind/src/Registers.hpp
5090

combine two == into one if

5094

combine < and >

5119

// and end with .

5267

combine into one if

libunwind/src/UnwindRegistersRestore.S
1369

misaligned

libunwind/src/UnwindRegistersSave.S
1302

misaligned

Ami-zhang marked 5 inline comments as done.Oct 29 2022, 12:31 AM
Ami-zhang added inline comments.
libunwind/src/Registers.hpp
5090

Ok, it's done.
Thanks for your time.

Address @MaskRay's comments.

tangyouling added inline comments.
libunwind/include/__libunwind_config.h
170

__loongarch64 was added just for compatibility, does the newly added code still go to use it?

libunwind/src/UnwindRegistersRestore.S
1302

__loongarch_frlen values may be 0, 32, 64, Bit-width of floating-point registers (0 if there is no FPU).
So can __loongarch_frlen be dropped?

Ami-zhang added inline comments.Oct 30 2022, 8:44 PM
libunwind/include/__libunwind_config.h
170

No, i make sure that __loongarch64 will not be used in the newly added code.
__loongarch64 is just to be compatible with the previous codes, and after that, the use of __loongarch_grlen can come in handy.

libunwind/src/UnwindRegistersRestore.S
1302

Yes, it can.

Ami-zhang updated this revision to Diff 471896.Oct 30 2022, 8:46 PM

Address @tangyouling's comments.

SixWeining added inline comments.Oct 31 2022, 6:24 PM
libunwind/src/Registers.hpp
5071

Only need to be defined if __loongarch_frlen == 64 or __loongarch_frlen != 0. Below you use == 64, so here better also use == 64.

Ami-zhang added inline comments.Oct 31 2022, 8:00 PM
libunwind/src/Registers.hpp
5071

Ok.
64-bits integer and float-point register implementation is only supported here.

Ami-zhang updated this revision to Diff 472205.Oct 31 2022, 8:02 PM

Address @SixWeining's comment.

Ami-zhang edited the summary of this revision. (Show Details)Oct 31 2022, 8:24 PM
xen0n added inline comments.Oct 31 2022, 8:24 PM
libunwind/include/__libunwind_config.h
172

This definition could come before the GRLen switch, just after __loongarch__ is matched? Because both GRLen=64 and the future GRLen=32 cases are LoongArch.

199

One space before define for consistency with neighboring code.

libunwind/src/Registers.hpp
5132

Do we really want the $ prefix? It's not for emitting assembly after all, and no architecture except MIPS does so. The RISCV port even features ABI names in their getRegisterName and I think it makes sense too, because it's actually clear we're dealing with a LP64* ABI here.

libunwind/src/UnwindRegistersRestore.S
1303

Those $r4 (at this position) could be just $a0 for clarity.

1369

nit: restore $a0 last for a nice short explanation of its placement.

1372

jr $ra is enough. (We might even use ret if we don't care about binutils 2.39.) And the comment provides no new information than the code so it's better removed.

libunwind/src/UnwindRegistersSave.S
1302

move $a0, $zero // UNW_ESUCCESS

Ami-zhang marked 3 inline comments as done.Nov 1 2022, 2:19 AM
Ami-zhang added inline comments.
libunwind/include/__libunwind_config.h
172

Ok, i'm for it.
I used to plan to do this as you said when the 32-bits is supported. But it's a better choice to do it now.
Thanks.

199

clang-format made it.

libunwind/src/Registers.hpp
5132

Yes, at least in LLVM LoongArch now.
As for $ prefix, it's different from different perspectives(https://reviews.llvm.org/D136436). And here, i followed the action of LLVM LoongArch. If this changes in the future, i'll get it done.

libunwind/src/UnwindRegistersRestore.S
1303

Ok, done.

Ami-zhang updated this revision to Diff 472239.Nov 1 2022, 2:21 AM

Address @xen0n's comments.

MQ-mengqing added inline comments.
libunwind/src/Registers.hpp
5109

It is not appreciated to return 0 directly. Generally we don't get dwarf NO.0 because $zero always is 0. As that it allows the user to use NO.0 as magic to do things like '.cfi_return_column $zero'.

5122

Same as ::getRegister, though it seems not change anything...

libunwind/src/UnwindRegistersRestore.S
1337

Neither uwn_setcontext nor uwn_getcontext use the 0 position. Besides, is there no needed to save or restore the fscr and the fcc?

Ami-zhang added inline comments.Nov 4 2022, 1:44 AM
libunwind/src/Registers.hpp
5109

Thanks for your advice.
I have modified it.

libunwind/src/UnwindRegistersRestore.S
1337

Yes, this takes no account of fcsr and fcc.
Maybe they will be added later.

Ami-zhang added inline comments.Nov 4 2022, 1:45 AM
libunwind/src/Registers.hpp
5122

ditto

Ami-zhang updated this revision to Diff 473163.Nov 4 2022, 1:49 AM

Address @MQ-mengqing's comments.

SixWeining accepted this revision.Nov 10 2022, 5:18 AM

LGTM but let's see if others have more inputs.

Result of ninja check-unwind:

********************
Unsupported Tests (5):
  llvm-libunwind-shared.cfg.in :: bad_unwind_info.pass.cpp
  llvm-libunwind-shared.cfg.in :: floatregister.pass.cpp
  llvm-libunwind-shared.cfg.in :: remember_state_leak.pass.sh.s
  llvm-libunwind-shared.cfg.in :: signal_unwind.pass.cpp
  llvm-libunwind-shared.cfg.in :: unwind_leaffunction.pass.cpp


Testing Time: 1.42s
  Unsupported: 5
  Passed     : 7
This revision was not accepted when it landed; it landed in state Needs Review.Nov 14 2022, 10:37 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Sorry, I missed this patch. The repeated instructions are difficult to read... I created D139368 for simplification.

Sorry, I missed this patch. The repeated instructions are difficult to read... I created D139368 for simplification.

It doesn't matter. And thanks for your simplification work.