[PATCH] D64835: [Xtensa 9/10] Add basic support of Xtensa disassembler.

[Sterling Augustine via Phabricator via llvm-commits]

saugustine added a comment.

This is largely mechanical, so mostly looks good with a minor nit.

Disassembly is likely to run into issues with invalid instructions in the stream because the llvm disassembler doesn't honor .xt.insn section notes. For example, the nop padding from D64831 <https://reviews.llvm.org/D64831> is going to trip this up if it runs into a single-byte pad, or the two-byte density nops, for that matter. This is one of the caveats of Xtensa's 2 and 3 byte instructions when you need to pad a single byte--there isn't any valid instruction to do that.

The alternative would be to never pad with illegal instructions. On a density machine, if you need a single-byte of padding, you can five-bytes (fetch width + remainder). On a non-density machine, it gets more complicated.

Still, this is not exactly a correctness issue, but it will make using this disassembler for real work tricky. Honoring .xt.insn sections is likely beyond the scope here.

Worse, the gnu assembler won't be able to properly disassemble these situations either, as this implementation doesn't generate .xt.insn sections.

I suppose people who care can use the gnu assembler and disassembler rather than llvm's built in version.



================
Comment at: llvm/lib/Target/Xtensa/Disassembler/XtensaDisassembler.cpp:171
+
+/// Read four bytes from the ArrayRef and return 24 bit data sorted
+/// according to the given endianness.
----------------
this only reads three bytes, does it not? And no need to comment about the endianness--it isn't supported.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64835/new/

https://reviews.llvm.org/D64835