Page MenuHomePhabricator

[RISCV 9/10] Add support for disassembly
ClosedPublic

Authored by asb on Aug 16 2016, 8:42 AM.

Details

Summary

Disassembler support allows for 'round-trip' testing, and rv32i-valid.s has been updated appropriately.

Diff Detail

Repository
rL LLVM

Event Timeline

asb updated this revision to Diff 68193.Aug 16 2016, 8:42 AM
asb retitled this revision from to [RISCV 9/10] Add support for disassembly.
asb updated this object.
asb added reviewers: theraven, jyknight.
asb added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Aug 16 2016, 2:43 PM
reames added a subscriber: reames.Aug 21 2016, 12:46 PM
reames added inline comments.
lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
44 ↗(On Diff #68193)

Style: Should these be in a header somewhere?

63 ↗(On Diff #68193)

Can this static cast be lifted through the call chain to the entry point? Or are each of these used directly from table gen? This is more than a bit messy and concerning.

71 ↗(On Diff #68193)

style: Is there a RegClass.max like value we could use here instead?

89 ↗(On Diff #68193)

Not quite sure what we're going for here, but sign extending an unsigned immediate seems a bit odd. Possibly a clarifying comment?

101 ↗(On Diff #68193)

Style: move this below the early return.

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
102 ↗(On Diff #68193)

We need to assert the size of the immediate don't we? Also, didn't we have a checked cast function in one of the previous patches already?

104 ↗(On Diff #68193)

Why the assert immediately followed by an unreachable

lib/Target/RISCV/RISCVInstrFormats.td
13 ↗(On Diff #68193)

What is this? I don't see it being used anywhere in this patch?

asb updated this revision to Diff 69356.Aug 26 2016, 5:31 AM
asb marked 5 inline comments as done.

Refresh to reflect change made in previous patches in the series, and address review comments from @reames.

lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
44 ↗(On Diff #68193)

Actually this is redundant as they're already in RISCVMCTargetDesc.h which is already include - removed. Thanks!

63 ↗(On Diff #68193)

I've lifted the cast to DecodeGPRRegisterClass, which is the entry-point from TableGen generated code, and in the end got rid of getReg entirely.

89 ↗(On Diff #68193)

The immediate value is extracted from the instruction and passed as a uint64_t. The assert checks that it the bitwidth is as we expect, but we can sign-extend it from the appropriate bit position. SignExtend64 is written for this exact purpose (sign extends the bottom N bits of the uint64_t that it is passed).

I've added a comment to clarify what is going on that hopefully makes it easier to follow? I've also added a comment in the new decodeSImmOperandAndLsl1 which is perhaps even more confusing. Do let me know any suggestions for making it clearer.

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
102 ↗(On Diff #68193)

I've now defined getImmOpValue rather than first defining getImm20OpValue only to change things in the next patch.

lib/Target/RISCV/RISCVInstrFormats.td
13 ↗(On Diff #68193)

Tablegenning the disassembler fails without it. I've stolen an explanatory comment from the AMDGPU backend:

// SoftFail is a field the disassembler can use to provide a way for
// instructions to not match without killing the whole decode process. It is
// mainly used for ARM, but Tablegen expects this field to exist or it fails
// to build the decode table.
theraven accepted this revision.Aug 26 2016, 7:14 AM
theraven edited edge metadata.
theraven added inline comments.
lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
111 ↗(On Diff #69356)

RISC-VC?

This revision is now accepted and ready to land.Aug 26 2016, 7:14 AM
jyknight added inline comments.Oct 3 2016, 2:23 PM
test/MC/RISCV/rv32i-valid.s
3 ↗(On Diff #69356)

Using the same file to test both directions seems like a nice method -- but strangely, almost unique amongst the tests for the backends.

I actually especially like the way it's done in test/MC/X86/mpx-encodings.s, where it uses the same CHECK: line for both the asm output of llvm-mc, and the output of llvm-objdump.

I'd suggest doing it that way, which would nicely resolve my comment in D23564 too.

asb updated this revision to Diff 74027.Oct 8 2016, 6:24 AM
asb edited edge metadata.
asb marked 2 inline comments as done.

Tests now check both assembly printing and disassembly. I've added a comment/TODO regarding decode of instructions outside of the RV32/RV64 core that might be 16-bit, 48bit, 64bit, ...

asb added inline comments.Oct 8 2016, 6:26 AM
test/MC/RISCV/rv32i-valid.s
3 ↗(On Diff #69356)

I went ahead with the CHECK-INST approach I described in D23564.

asb updated this revision to Diff 74226.Oct 11 2016, 3:55 AM

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors.

asb updated this revision to Diff 74380.Oct 12 2016, 7:56 AM

Fix rv32i-valid.s, so CHECK-INST is used rather than CHECK-DIS.

asb updated this revision to Diff 88332.Feb 14 2017, 2:03 AM

The previous version of this patch made the mistake when decoding a GPR of assuming you can use the parsed RegNo as an index into the register class. In the general case, a register class will have the registers in preferred allocation order, meaning this will fail. Instead, we have a GPRDecoderTable to select the appropriate register based on the parsed RegNo.

Razer6 added a subscriber: Razer6.Feb 14 2017, 6:28 AM
This revision was automatically updated to reflect the committed changes.