This is an archive of the discontinued LLVM Phabricator instance.

[mips][mips64r6] Add b[on]vc
ClosedPublic

Authored by dsanders on May 14 2014, 5:40 AM.

Details

Summary

This required me to implement the disassembler for MIPS64r6 since the encodings
are ambiguous with other instructions. This in turn revealed a few
assembly/disassembly bugs which I have fixed.

  • da[ht]i only take two operands according to the spec, not three.
  • DecodeBranchTarget2[16] correctly handles wider immediates than simm16
    • Also made non-functional change to DecodeBranchTarget and DecodeBranchTargetMM to keep implementation style consistent between them.
  • Difficult encodings are handled by a custom decode method on the most general encoding in the group. This method will convert the MCInst to a different opcode if necessary.

DecodeBranchTarget is not currently the inverse of getBranchTargetOpValue
so disassembling some branch instructions emit incorrect output. This seems
to affect branches with delay slots on all MIPS ISA's. I've left this bug
for now and temporarily removed the check for the immediate on
bc[12]eqz/bc[12]nez in the MIPS32r6/MIPS64r6 tests.

jialc and jic crash the disassembler for some reason. I've left these
instructions commented out for the moment.

Depends on D3760

Diff Detail

Event Timeline

dsanders updated this revision to Diff 9386.May 14 2014, 5:40 AM
dsanders retitled this revision from to [mips][mips64r6] Add b[on]vc.
dsanders updated this object.
dsanders edited the test plan for this revision. (Show Details)
vmedic edited edge metadata.May 15 2014, 1:59 AM

When decoding immediate operands the sign extension is done first and then left shift by two. I'm not sure that left shift is arithmetic and will keep the sign.

Left shift can only lose the sign bit through overflow. In this case the sign extend guarantees that there are 48-bits of sign extension and we only shift out 2 bits. It's therefore not possible to overflow.

dsanders updated this revision to Diff 9438.May 15 2014, 6:40 AM
dsanders edited edge metadata.

Actually fix da[ht]i.

dsanders updated this revision to Diff 9611.May 20 2014, 5:39 AM

Refresh after rebase and fixed bugs related to code added on the trunk (e.g.
the bcczalc instructions are now implemented so the custom decoder needed
updating)

dsanders updated this object.May 20 2014, 5:39 AM
dsanders updated this object.
vmedic accepted this revision.May 22 2014, 3:41 AM
vmedic edited edge metadata.

DecodeAddiGroupBranch and DecodeDaddiGroupBranch in the comment mention ADDI at the beginning(copy/paste?). Other that that, it looks good to me.

This revision is now accepted and ready to land.May 22 2014, 3:41 AM

DecodeAddiGroupBranch and DecodeDaddiGroupBranch in the comment mention ADDI at the beginning(copy/paste?).

It was deliberate but I see why the comments are confusing. I'll make it clearer that ADDI/DADDI only exist in MIPS32r5/MIPS64r5 and earlier.

dsanders closed this revision.May 22 2014, 4:30 AM