[Lldb-commits] [PATCH] D12356: [MIPS64] Emulate MSA branch instructions

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

I don't know too much about mips so I haven't checked if the emulation is actually correct but the general concept looks good to me. I added a few comments inline but they are mostly suggestions what you might want to consider.


================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:553-562
@@ -476,2 +552,12 @@
         { "BC1ANY4T",   &EmulateInstructionMIPS64::Emulate_BC1ANY4T,    "BC1ANY4T cc, offset"       },
+        { "BNZ_B",     &EmulateInstructionMIPS64::Emulate_BNZB,        "BNZ.b wt,s16"             },
+        { "BNZ_H",     &EmulateInstructionMIPS64::Emulate_BNZH,        "BNZ.h wt,s16"             },
+        { "BNZ_W",     &EmulateInstructionMIPS64::Emulate_BNZW,        "BNZ.w wt,s16"             },
+        { "BNZ_D",     &EmulateInstructionMIPS64::Emulate_BNZD,        "BNZ.d wt,s16"             },
+        { "BZ_B",      &EmulateInstructionMIPS64::Emulate_BZB,         "BZ.b wt,s16"              },
+        { "BZ_H",      &EmulateInstructionMIPS64::Emulate_BZH,         "BZ.h wt,s16"              },
+        { "BZ_W",      &EmulateInstructionMIPS64::Emulate_BZW,         "BZ.w wt,s16"              },
+        { "BZ_D",      &EmulateInstructionMIPS64::Emulate_BZD,         "BZ.d wt,s16"              },
+        { "BNZ_V",     &EmulateInstructionMIPS64::Emulate_BNZV,        "BNZ.V wt,s16"              },
+        { "BZ_V",      &EmulateInstructionMIPS64::Emulate_BZV,         "BZ.V wt,s16"               },
     };
----------------
(nit): Indentation

================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3138-3140
@@ +3137,5 @@
+    bool success = false, branch_hit = true;
+    uint32_t wt;
+    int64_t offset, pc, target;
+    RegisterValue reg_value;
+    uint8_t * ptr = NULL;
----------------
Please initialize these variables

================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3160
@@ +3159,3 @@
+            case 1:
+                if((*ptr == 0 && bnz) || (*ptr != 0 && !bnz) )
+                    branch_hit = false;
----------------
You can possibly write it in the following way, but I am not sure which one is the cleaner:

```
if((*ptr == 0) == bnz)
    break;
```

================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3187
@@ +3186,3 @@
+    Context context;
+    context.type = eContextInvalid;
+
----------------
Using eContextInvalid as context type is fine for single stepping but if you want these instructions to be handled correctly during emulation based stack unwinding then you have to use eContextAbsoluteBranchRegister or eContextRelativeBranchImmediate with the correct data (it is used by the branch following code in the unwinder)

================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3211-3213
@@ +3210,5 @@
+    bool success = false;
+    uint32_t wt;
+    int64_t offset, pc, target;
+    llvm::APInt wr_val;
+    llvm::APInt fail_value = llvm::APInt::getMaxValue(128);
----------------
Please initialize these variables

================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3230
@@ +3229,3 @@
+
+    if((llvm::APInt::isSameValue(zero_value, wr_val) && !bnz) || (!llvm::APInt::isSameValue(zero_value, wr_val) && bnz))
+        target = pc + offset;
----------------
You can possibly write it in the following way, but I am not sure which one is the cleaner:

```
if(llvm::APInt::isSameValue(zero_value, wr_val) != bnz)
    ...
```

================
Comment at: source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp:3236
@@ +3235,3 @@
+    Context context;
+    context.type = eContextInvalid;
+
----------------
Using eContextInvalid as context type is fine for single stepping but if you want these instructions to be handled correctly during emulation based stack unwinding then you have to use eContextAbsoluteBranchRegister or eContextRelativeBranchImmediate with the correct data (it is used by the branch following code in the unwinder)


Repository:
  rL LLVM

http://reviews.llvm.org/D12356