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

[Hui Li via Phabricator via lldb-commits]

lh03061238 added inline comments.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:42
        "bnez rj, offs21"},
+      {0xfc000000, 0x48000000, &EmulateInstructionLoongArch::EmulateBCXXZ,
+       "bceqz/bcnez cj, offs21"},
----------------
SixWeining wrote:
> 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
My understanding of this opcode is not accurate,  This will be modified.
Thanks for pointing out.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:303
 
+// bceqz   cj, offs21
+// if CFR[cj] == 0:
----------------
SixWeining wrote:
> A single space is enough.
will be modified， thanks


================
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);
----------------
SixWeining wrote:
> Is this number of FPRs? Would it be changed in future when we support vertor registers? Adding some comment may help future readers.
It's a little complicated here,  I will use “Bits32(inst, 7, 5) + fpr_fcc0_loongarch”  to make it clearer.
will be modified， thanks


================
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));
+}
----------------
SixWeining wrote:
> 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`.
This will be modified.
Thanks for pointing out.


================
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);
----------------
SixWeining wrote:
> Ditto.
will be modified， thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140759