[Lldb-commits] [PATCH] D140759: [LLDB][LoongArch] Add FP branch instructions for EmulateInstructionLoongArch

[Lu Weining via Phabricator via lldb-commits]

SixWeining added inline comments.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:42
        "bnez rj, offs21"},
+      {0xfc000000, 0x48000000, &EmulateInstructionLoongArch::EmulateBCXXZ,
+       "bceqz/bcnez cj, offs21"},
----------------
It should be 0xfc000300. BCEQZ and BCNEZ should be seperated.
See https://github.com/loongson/LoongArch-Documentation/blob/main/docs/LoongArch-Vol1-EN/table-of-instruction-encoding.adoc


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:303
 
+// bceqz   cj, offs21
+// if CFR[cj] == 0:
----------------
A single space is enough.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:312
+  uint32_t bcxxz = (inst >> 8) & 0b11;
+  uint32_t cj = Bits32(inst, 7, 5) + fpr_first_loongarch + 32;
+  uint64_t pc = ReadPC(&success);
----------------
Is this number of FPRs? Would it be changed in future when we support vertor registers? Adding some comment may help future readers.


================
Comment at: lldb/unittests/Instruction/LoongArch/TestLoongArchEmulator.cpp:157
+static uint32_t BCEQZ(uint8_t cj, int32_t offs21) {
+  return EncodeBCZcondType(0b010010, cj, 0b00, uint32_t(offs21));
+}
----------------
The opcode is 8 bits `0b01001000`. See: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/LoongArch/LoongArchFloat32InstrInfo.td#L109

I think you can remove the third argument `bcxxz`.


================
Comment at: lldb/unittests/Instruction/LoongArch/TestLoongArchEmulator.cpp:256
+  tester->fpr.fcc = cj_val;
+  // bc<cmp>z  fcc0, (256)
+  uint32_t inst = encoder(0, 256);
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140759