labath added a comment.
Some more comments (and questions) from me. Sorry about the back-and-forth.
It's taking me a while to wrap my head around this, and as I start to
understand things, new questions arise...
================
Comment at:
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:282-290
+ if (!m_per_vq_reg_infos.count(sve_vq)) {
+
+ m_per_vq_reg_infos.insert(std::make_pair(
+ sve_vq, std::vector<lldb_private::RegisterInfo>(
+ g_register_infos_arm64_sve_le,
+ g_register_infos_arm64_sve_le + m_register_info_count)));
+
----------------
I'd probably try relying on the fact that `operator[]` constructs an empty
vector, and that the empty vector is not a valid register info value. Something
like:
```
std::vector<RegisterInfo> &new_reg_info = m_per_vq_reg_infos[sve_vq];
if (new_reg_info.empty()) {
new_reg_info = llvm::makeArrayRef(g_register_infos_arm64_sve_le,
m_register_info_count);
...
}
m_register_info_p = new_reg_info.data();
```
That would make this less of a mouthful and also avoid looking up the same key
in the map three or four times.
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:17-22
+enum class SVE_STATE {
+ SVE_STATE_UNKNOWN,
+ SVE_STATE_DISABLED,
+ SVE_STATE_FPSIMD,
+ SVE_STATE_FULL
+};
----------------
If this is going to be a class enum (which I think is a good idea), then
there's no need to repeat the type name in the actual enumerators. It also
seems like this could/should follow the lldb naming conventions
(SveState::Disabled, etc).
================
Comment at: lldb/source/Plugins/Process/Utility/lldb-arm64-register-enums.h:261
+ k_num_fpr_registers_arm64 = k_last_fpr_arm64 - k_first_fpr_arm64 + 1,
+
+ k_first_sve_arm64 = exc_far_arm64,
----------------
I guess these changes should be reverted now?
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106
+uint32_t
+RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const {
+ uint32_t sve_offset = 0;
+ if (m_sve_state == SVE_STATE::SVE_STATE_FPSIMD) {
+ if (IsSVEZ(reg_num))
+ sve_offset = (reg_num - GetRegNumSVEZ0()) * 16;
+ else if (reg_num == GetRegNumFPSR())
----------------
I'm confused by this function. If I understant the SVE_PT macros and the logic
in `RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos` correctly, then they
both seem to encode the same information. And it seems to me that this function
should just be `reg_infos[reg_num].offset - some_constant`, which is the same
thing that we're doing for the arm FP registers when SVE is disabled, and also
for most other architectures too.
Why is that not the case? Am I missing something? If they are not encoding the
same thing, could they be made to encode the same thing?
================
Comment at:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h:17
+#include "Plugins/Process/Utility/LinuxPTraceDefines_arm64sve.h"
+
----------------
Please group the header next to other `Plugins/Process/Utility` header
================
Comment at:
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py:326-327
+ #@skipIf(triple='^mips')
+ #@skipIfLLVMTargetMissing("AArch64")
+ def test_aarch64_sve_regs(self):
----------------
What's up with these?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77047/new/
https://reviews.llvm.org/D77047
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
