[Lldb-commits] [PATCH] D138407: [LLDB] Add LoongArch register definitions and operations

Wed Nov 23 23:42:39 PST 2022

seehearfeel added inline comments.

================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h:21
+#define GPR_OFFSET(idx) ((idx)*8 + 0)
+#define FPR_OFFSET(idx) ((idx)*8 + sizeof(RegisterInfoPOSIX_loongarch64::GPR))
+#define FCC_OFFSET(idx) ((idx)*1 + 32 * 8 + sizeof(RegisterInfoPOSIX_loongarch64::GPR))
----------------
SixWeining wrote:
> I'm not sure whether you could use `RegisterInfoPOSIX_loongarch64` in this file directly because I think this file is a `common` file. What do you think? @DavidSpickett
Maybe it is better to define them in RegisterInfoPOSIX_loongarch64.cpp, let me modify it, thank you.

================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h:98
+    DEFINE_GPR64_ALT(r20, t8, LLDB_INVALID_REGNUM),
+    DEFINE_GPR64_ALT(r21, u0, LLDB_INVALID_REGNUM),
+    DEFINE_GPR64_ALT(r22, fp, LLDB_REGNUM_GENERIC_FP),
----------------
xen0n wrote:
> SixWeining wrote:
> > `u0` is a unknown alias. Could we just use `DEFINE_GPR64`?
> FYI the `u0` name is [[ https://www.kernel.org/doc/html/latest/loongarch/introduction.html | a non-standard alias only seen in the Linux kernel ]]. It should be harmless to just support `r21` but not `u0`, much like how we don't support `v0/v1` any more.
> 
> While at it, `s9` in addition to `fp` may be supported too. (Arguably `s9` is a better description of `r22` than `fp`, because FP usage can be disabled while generating code, in which case it's just another ordinary callee-saved register. But it seems some people believe so deeply that this usage is acceptable that the name persisted into the final ABI document...)
OK,  according to Register Convention in LoongArch ELF ABI specification
https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_register_convention

let me remove the alias u0 for r21, use fp and s9 for r22, thank you.

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

https://reviews.llvm.org/D138407