Add instruction subset for the ARC backend
Needs ReviewPublic

Authored by tatyana-krasnukha on Sep 18 2017, 10:42 AM.

Details

Reviewers
petecoup
Summary

Add instruction subset required for disassembling:

  • All compact (16-bit) instruction formats;
  • A couple of 32-bit instructions;
  • Ability to print symbolic information for branch and call instructions.

clang-format is applied.

Diff Detail

Repository
rL LLVM

Hi Tatyana,

Thanks for working on this!
Can you add disassembly tests for the new 16-bit short instructions that you added here?
There are a few places in the ARCDisassembler code where you use 'auto' instead of standard scalar types (unsigned/uint64_t/etc.). Can you use these types instead of auto?
You create a new namespace (arc::inst_decoder) in the Disassembler. This looks different than the rest of the Disassemblers, can you use static routines instead?
Thanks again!

ARC/ARCInstrInfo.td
217 ↗(On Diff #115671)

These aren't right, please remove these MultiPat<sub, SUB[123]...> lines. You can keep the defm SUB[123]: ... lines.

ARC/Disassembler/ARCDisassembler.cpp
128 ↗(On Diff #115671)

Would DecodeGBR32ShortRegister be a better name?

172 ↗(On Diff #115671)

I think static_cast is better than reinterpret_cast here?

Hi Pete, thank you for review!

I'll do suggested corrections as soon as I'll have tests written.
Speaking of static and namespaces, I don't like this C-style code, but if you insist...)

ARC/ARCInstrInfo.td
217 ↗(On Diff #115671)

Ok.

ARC/Disassembler/ARCDisassembler.cpp
128 ↗(On Diff #115671)

Yes, probably it is...

172 ↗(On Diff #115671)

static_cast could lead to the mistaken belief that this type cast is safe, whereas reinterpret_cast is more conspicuous and reminding that the code requires changes)

Hello Tatyana,

I am mostly going with the style/conventions observed in the other backends/disassemblers. Can we use these current conventions for this patch? I feel like ARCDisassembler doesn't need many changes to enable the new instructions.
That will let us focus on the main new material here (new .td definitions/16-bit disassembly support). Perhaps suggestions on when do use C++ namespaces and MCDisassembler* instead of void* that would apply for all disassemblers could be proposed in a separate patch?

So, I view the goal of this patch is to provide enough 16-bit instruction format information so we can disassemble all 16-bit instructions with these formats.
Thanks again!

ARC/ARCInstrFormats.td
882 ↗(On Diff #115671)

For consistency, can we change asmstr#"stuff" -> !strconcat(asmstr, "stuff")?

ARC/ARCInstrInfo.td
421 ↗(On Diff #115671)

If we map bits 10-8,4-3 to the g register, do we need the custom decoder method anymore?

423 ↗(On Diff #115671)

This doesn't need to be strconcat.

ARC/Disassembler/ARCDisassembler.cpp
286 ↗(On Diff #115671)

See my earlier comment about mapping the correct bits for the g register. I don't think this custom decoder routine is needed.

368 ↗(On Diff #115671)

Please remove the braces from this conditional.

ARC/InstPrinter/ARCInstPrinter.cpp
105 ↗(On Diff #115671)

This reads a bit easier as 2 lines, and is how other printers do this:
OS << "0x";
OS.write_hex(CE->getValue());

Added tests, fixed ARCDisassembler.cpp according suggestions.

ARC/ARCInstrInfo.td
421 ↗(On Diff #115671)

Yes, it is required to handle such cases as limm data operand or no destination (MOV_S 0,h and MOV_S 0,limm). I haven't found the way to implement it in .td

Applied fixes from 2nd comment.

Forgot to change decoder method name.

Hello Tatyana,

Thanks again for the work here.
I applied this patch, and am getting build errors (DecodeFromCyclicRange, ReadField used before declared, others).
Assuming these declarations get moved to the right places, my only remaining question is how others feel about the ReadField/ReadFields variadic templates.

lib/Target/ARC/Disassembler/ARCDisassembler.cpp
273

This makes the users more compact, but it seems less readable.

Thanks for comment, Pete!

Fixed the order of declarations and using of functions.
Removed variadic template for better readability.

tatyana-krasnukha marked an inline comment as done.Thu, Oct 12, 9:22 AM

I'm getting a few errors when running these tests now?

misc.txt:25:10: error: expected string not found in input
# CHECK: bl -2028
         ^
<stdin>:9:2: note: scanning from here
 bl -1996

br.txt:6:10: error: expected string not found in input
# CHECK: brlo %r10, %r4, -112
         ^
<stdin>:3:2: note: scanning from here
 brlo %r10, %r4, -108
 ^

compact.txt:81:10: error: expected string not found in input
# CHECK: b_s 256
         ^
<stdin>:28:2: note: scanning from here
 b_s 316
 ^

Corrupted it by merging...