DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.
This is all very repetitive, but that's to be expected. Whatever you can do to
reduce boilerplate in future patches would be great. For example, can 32 and 64
bit variants of some instructions be emulated in the same way? Perhaps with a
template parameter for the return types.
This LGTM with a few comments you can do/not do/keep in mind for future patches.
================
Comment at:
lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:276
+ return false;
+}
+
----------------
All these could be of the form:
```
bool EmulateInstructionLoongArch::EmulateBGEU(uint32_t inst) {
return IsLoongArch64()) ? EmulateBGEU64(inst) : false;
}
```
Generally I'd advise coming up with a different way of doing this, that checks
up front that you won't call `EmulateBGEU` and friends when you are emulating
32 bit. Perhaps some table lookup first.
That said perhaps you intend to replace the `else` here with a call to
`EmulateBGEU32` in future. Up to you how to structure that.
In any case I'd slim down these functions as suggested for now.
================
Comment at:
lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:368
+ bool success = false;
+ uint64_t pc = ReadPC(&success);
+ if (!success)
----------------
This is what I mean about using optionals.
```
std::optional<uint64_t> pc = ReadPC(&success);
if (!pc)
return false;
EmulateInstruction::Context ctx;
if (!WriteRegisterUnsigned(ctx, eRegisterKindLLDB, gpr_r1_loongarch, *pc + 4))
return false;
```
It's a small saving and ok you have to `*` some places but when this stuff gets
more complicated it can help.
Anyway, up to you but keep it in mind as things get more complicated.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139833/new/
https://reviews.llvm.org/D139833
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
