[libcxx-commits] [PATCH] D137010: [libunwind][LoongArch] Add 64-bit LoongArch support

[Limin Zhang via Phabricator via libcxx-commits]

Ami-zhang marked 3 inline comments as done.
Ami-zhang added inline comments.


================
Comment at: libunwind/include/__libunwind_config.h:172
+#if __loongarch_grlen == 64
+#define _LIBUNWIND_TARGET_LOONGARCH 1
+#define _LIBUNWIND_CONTEXT_SIZE 65
----------------
xen0n wrote:
> 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.
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.


================
Comment at: libunwind/include/__libunwind_config.h:199
 # define _LIBUNWIND_TARGET_S390X 1
+#define _LIBUNWIND_TARGET_LOONGARCH 1
 # define _LIBUNWIND_CONTEXT_SIZE 167
----------------
xen0n wrote:
> One space before `define` for consistency with neighboring code.
`clang-format` made it.


================
Comment at: libunwind/src/Registers.hpp:5132
+  case UNW_REG_IP:
+    return "$pc";
+  case UNW_REG_SP:
----------------
xen0n wrote:
> 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.
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.




================
Comment at: libunwind/src/UnwindRegistersRestore.S:1303
+# if __loongarch_frlen == 64
+  fld.d    $f0, $r4, (8 * 33 + 8 * 0)
+  fld.d    $f1, $r4, (8 * 33 + 8 * 1)
----------------
xen0n wrote:
> Those `$r4` (at this position) could be just `$a0` for clarity.
Ok, done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137010/new/

https://reviews.llvm.org/D137010