[PATCH] D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Hi Sameer. Some notes primarily from reviewing the patterns and testing so far:

- In the RISCV backend we order tablegen instruction definitions to match the ordering in the instruction set manual (which allows easy cross-reference, makes it harder to miss one) and typically order codegen patterns grouped by function. For the case of the CompressPat, I'd probably keep them ordered by the instruction set manual (page 82 in the spec, like the instruction definitions) as each pattern is so mechanical. This makes it easier at a glance to see that every compressed instruction has a matching transformation. The basic MC tests should be ordered to reflect this too. I know this is probably a tedious change to make, but I do think a little effort in the formatting here has an impact on maintainability.
- Related to the above, grouping the 'commuted and repeated patterns' after the normal patterns makes it easier to miss one and a little more effort to check it's there. I'd always group patterns for the same target compressed instruction together.
- Are the CHECK-BYTES lines worthwhile? I'm fairly happy making the assumption that the encoding printed by the InstPrinter is reflected in the output object (or at least, I'm not sure it's worth adding an extra line for every single check to ensure this)
- For other MC tests, we've checked the instructions common to rv32 and rv64 in a single file. See e.g. rv32c-valid.s, rv32c-only-valid.s etc
- This compression transformation is intended to use the same code path to support a range of use cases: .s->.o, .ll->.o, showing aliases in disassembly, compression of inline asm. This patch only contains tests for the assembler and disassembler paths. Given that the compression transformation is implemented as an MCInst->MCInst transformation, it's fine for the exhaustive tests to live in test/MC/RISCV, but we should at least have some sanity checks that the codegen compression path is working (including with inline asm).



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:16
 #include "RISCVMCTargetDesc.h"
+#include "RISCVELFStreamer.h"
 #include "llvm/BinaryFormat/ELF.h"
----------------
RISCVELFStreamer.h should go first, as it is the counterpart to RISCVELFStreamer.cpp ('[[ https://llvm.org/docs/CodingStandards.html#main-module-header | main module header ]]')


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:533
+let Predicates = [HasStdExtC] in {
+// FIXME: missing Q instructions, nop, ebreak.
+def : CompressPat<(ADD GPRNoX0:$rs1, GPRNoX0:$rs1, GPRNoX0:$rs2),
----------------
It would be less ambigious to say 'missing RV128C-only instructions', so as not to be confused with the Q instruction set extension (which has no compressed forms of its instructions anyway). RV128 of course isn't final/frozen yet.

Why are nop and ebreak not yet supported? I could understand nop may not work due to lack of support for integer constants, but ebreak -> c.ebreak should be easy specify? Given they're the only two missing instructions, it would be nice to support them.


https://reviews.llvm.org/D42780