[Lldb-commits] [PATCH] D82863: [LLDB] Add support to resize SVE registers at run-time

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D82863#2375525 <https://reviews.llvm.org/D82863#2375525>, @omjavaid wrote:

> GDB does not have pseudo registers as part of xml register description. GDB is managing pseudo registers on client side and independent of rest of the register set. In case of SVE Z registers, GDB describes a composite type kind of a union of (Z, V, D and S) which helps in giving registers a view in terms of V, S and D which are not actually register.
>
> As invalidate_regs and value_regs are totally LLDB specific so we can put information about LLDB speicific pseudo registers in those lists which we actually do by the way. Taking advantage of that I have S, V and D registers in invalidate_reg list of a Z register which share the same starting offset as the corresponding Z register but are size restricted to 4, 8 or 16 bytes.
>
> For the sake of clarity here is the scheme:
>
> We have two types of registers:
>
> 1. Primary registers
>
> Includes 64 bit GPRs Xn regs, PC etc
> Also includes V registers for all non-sve targets
> Includes Z, P, FFR and VG for SVE targets.
>
> All primary registers have value_regs list set to nullptr
> All primary registers have invalidate_regs list which is a list of registers which share same starting offset as the corresponding primary registers.
>
> 2. Pseudo registers
>
> Includes 32 bit GPRs Wn regs
> Includes D and S registers for all non SVE targets
> Also includes V, D and S registers for all SVE targets
>
> All pseudo register have a value register which can be found in value_regs list of that register.
> All pseudo registers have invalidate_regs list which is a list of registers which share same starting offset as the corresponding primary registers.
>
> On start of debug session we exchange qRegisterInfo or target XML packet registers on client side are numbered in sequence they are received in qRegisterInfo or their placement location in XML description. We receive register offset field populated in case we are talking to lldb-server.

Up to here, everything makes sense to me.  Thanks for summarizing that.

> This offset will be ignored in case of AArch64 target with SVE and it will be recalculated in DynamicRegisterInfo::Finalize function.

Didn't we say we were going to make lldb-server stop sending offsets in the SVE (or even aarch64 in general) case?

> Moreover, whenever we stop we get register VG in expedited registers list. we call GDBRemoteRegisterContext::AArch64SVEReconfigure function which will evaluate if we need to update offsets. In case VG has been updated whole register list will be traversed in sequence and new offsets will be assigned according to updated register sizes.

Ideally, I'd want to structure this in a way so that it does not depend on whether lldb-server expedites any particular register. The idea is, that after every stop, the client would check the value of the VG register (and update stuff if it changed). If the register was expedited, then this access would be satisfied from the expedited cache. But if the server did not send it for any reason, (and this is structured correctly) the client should just transparently request the updated register value from the server.



================
Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:543
 
+  // On AArch64 architecture with SVE support enabled we set offsets on client
+  // side based on register size and its position in m_regs.
----------------
We should already have code somewhere which sets the register offsets in case the server did not provide them. We should merge the two. Ideally, this wouldn't even require any special logic, as the server will just not send any offsets for SVE.


================
Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:717
   m_reg_data_byte_size = 0;
+  m_per_thread_reginfo = false;
   m_finalized = false;
----------------
This fields is out of place here, as this class doesn't know anything about threads. I guess what it really means is "can the size of these registers be changed at runtime".

A natural consequence of this would then be that its users would need to keep a copy of each class for each thread...


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp.rej:1
+--- lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
++++ lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
----------------
?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp:734
 
+bool GDBRemoteRegisterContext::AArch64SVEReconfigure(void) {
+  if (!m_reg_info_sp)
----------------
`(void)` is a c-ism.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1769-1770
+      const ArchSpec &arch = GetTarget().GetArchitecture();
+      if (arch.IsValid() && (arch.GetMachine() == llvm::Triple::aarch64 ||
+                             arch.GetMachine() == llvm::Triple::aarch64_be)) {
+        GDBRemoteRegisterContext *reg_ctx_sp =
----------------
`arch.GetTriple().isAArch64()` covers both.


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

https://reviews.llvm.org/D82863