This is an archive of the discontinued LLVM Phabricator instance.

[MIPS64] Emulate MSA branch instructions
ClosedPublic

Authored by slthakur on Aug 26 2015, 12:09 AM.

Details

Summary

This patch adds MSA branch instruction emulation for MIPS64.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 33184.Aug 26 2015, 12:09 AM
slthakur retitled this revision from to [MIPS64] Emulate MSA branch instructions.
slthakur updated this object.
slthakur added reviewers: jaydeep, clayborg.
slthakur set the repository for this revision to rL LLVM.
jaydeep requested changes to this revision.Aug 26 2015, 12:18 AM
jaydeep edited edge metadata.
jaydeep added inline comments.
source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
3117

This should be pc + 8. If branch is not taken then we need to skip the delay slot.

This revision now requires changes to proceed.Aug 26 2015, 12:18 AM

You added 10 new function to the emulator where the first 8 functions and the last 2 functions are almost identical. I know that the rest of the MIPS64 instruction emulator is having the same issue but I would like to see these functions to be merged into a smaller number of functions to remove code duplication.

I see 2 different approach to do it and I am not sure which one is better in this case, but both of them improves the situation by a lot.

  • Write only 2 function (first 8, last 2) and that function then reads out the condition and the sizes from the MCInst object. I would prefer this approach, but I am not sure how difficult it is to read out data from an MCInst
  • Keep the 10 function for the 10 different instruction, but make these functions to call a function (one for the first 8 and one for the last 2) with passing in the constants required by the specific instruction.

Both approach would reduce the amount of code in this class and help a lot in reducing the maintenance cost with removing a lot of duplicated code.

clayborg resigned from this revision.Aug 26 2015, 9:53 AM
clayborg removed a reviewer: clayborg.

I will defer to MIPS experts here. You might thing about adding tberghammer as a reviewer.

slthakur updated this revision to Diff 33323.Aug 27 2015, 7:03 AM
slthakur edited edge metadata.

Addressed review comments

tberghammer edited edge metadata.Aug 27 2015, 7:36 AM

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.

source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
553–562

(nit): Indentation

3138–3140

Please initialize these variables

3160

You can possibly write it in the following way, but I am not sure which one is the cleaner:

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

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)

3211–3213

Please initialize these variables

3230

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)
    ...
3236

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)

slthakur updated this revision to Diff 33567.EditedAug 31 2015, 4:23 AM
slthakur edited edge metadata.

Addressed review comments

slthakur marked 7 inline comments as done.Aug 31 2015, 4:42 AM
slthakur added inline comments.
source/Plugins/Instruction/MIPS64/EmulateInstructionMIPS64.cpp
3157

The former one is more readable.

3184

I have set the type to eContextRelativeBranchImmediate for now we are using these instructions for single stepping only.

3227

The former one is more readable.

3233

I have set the type to eContextRelativeBranchImmediate for now we are using these instructions for single stepping only.

slthakur updated this revision to Diff 33668.Aug 31 2015, 11:41 PM
slthakur marked 4 inline comments as done.

Corrected code indent and initialized wr_val correctly.

tberghammer accepted this revision.Sep 1 2015, 2:19 AM
tberghammer edited edge metadata.

LGTM

jaydeep accepted this revision.Sep 1 2015, 9:59 PM
jaydeep edited edge metadata.

Looks good to me

This revision is now accepted and ready to land.Sep 1 2015, 9:59 PM
slthakur closed this revision.Sep 2 2015, 9:52 PM

Committed in revision 246745