[Lldb-commits] [PATCH] D156687: [lldb][AArch64] Add kind marker to ReadAll/WriteALLRegisterValues data

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

In D156687#4628743 <https://reviews.llvm.org/D156687#4628743>, @DavidSpickett wrote:

> First thing to note is that WriteRegister also behaves this way, but there it is more appropriate because it updates only part of the buffer before writing it out in its entirety. Useful to know where the pattern came from though.
>
> You would need roughly the following per `WriteXYZ`:
>
>   -      error = WriteTLS();
>   +      error = WriteTLS(src);
>   
>   -Status NativeRegisterContextLinux_arm64::WriteTLS() {
>   +Status NativeRegisterContextLinux_arm64::WriteTLS(const void* src/*=nullptr*/) {
>   
>   -  ioVec.iov_base = GetTLSBuffer();
>   +  ioVec.iov_base = src ? const_cast<void*>(src) : GetTLSBuffer();
>   
>   -  Status WriteTLS();
>   +  Status WriteTLS(const void* src=nullptr);
>
> We can assume that the buffer is the same as the data to be written back if it's something static like TLS. For SVE/SME, we would have resized our buffer first, so it holds there too.
>
> The added complexity isn't that much but I think it adds more thinking time for a developer than it potentially saves in copying time. I already feel like the separate `xyz_is_valid` is enough to think about and having a potential second data source just adds to that load.
>
> From the header docs it seems I was right that this is used primarily for expression evaluation:
>
>   // These two functions are used to implement "push" and "pop" of register
>   // states.  They are used primarily for expression evaluation, where we need
>   // to push a new state (storing the old one in data_sp) and then restoring
>   // the original state by passing the data_sp we got from ReadAllRegisters to
>   // WriteAllRegisterValues.
>
> Which you would be doing a lot of in a formatter for example, but you'd get better savings implementing a more efficient packet format to do all that at once, I guess.
>
> QSaveRegisterState / QRestoreRegisterState packets call it as part of expression evaluation, though in theory it's not always that. That's an lldb extension anyway so we're in control of it at least. In theory this could be used to restore state that is not just the previous state but I don't know how you'd trigger that from lldb.
>
> The other use is `NativeProcessLinux::Syscall` which is sufficiently rare we can ignore that.
>
> I did do a very rough benchmark where I printed the same expression 2000 times, so each one is doing a save/restore. Once with the code in this review right now, and again with this potential optimisation added to GPR/FPR/TLS (I'm on a Mountain Jade machine without SVE). Caveat shared machine, made up benchmark, etc. but all runs of both hovered between 16 and 17 seconds. Neither seemed to be consistently lower or higher than the other. Doesn't mean this isn't a speedup in isolation but if it is, it's dwarfed by the syscalls and packets sent back and forth.

I mostly agree with what you have above. I was only thinking about ever increasing size of this buffer and thought if we can find a way around duplication. Most probably we ll never have SVE/SME enabled on a slow/small machine to bother us. The only area of concern could be a LLDB running on a resource constrained container but we can ignore that for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156687