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

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added a comment.

In https://reviews.llvm.org/D42780#1023507, @asb wrote:

> 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.


Thank you for the comment, Alex.

Th problem with this is that the instruction are not ordered base on the required target features, C.FLW is an rv32 only instruction but C.LD is rv64/128 then followed by C.FSD. This causes  the listing to be full of 
let Predicates = [HasStdExtC] in {
}
if we don't group the instructions based on target features. I think this will make the listing a bit messy, what do you think?

> - 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.

I agree and I think this is doable, I'll update the patch accordingly.

> - 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)

CHECK-BYTES" checks for the path coming from llvm-objdump (1.o --> 1.s) , the other check is for the path coming through llvm-mc (1.s   ---> 1.o). So this check that the places where comperss\uncompress are added are not breaking functionality for the llvm-objdump path.

> - 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

So are you suggesting combing   test/MC/RISCV/compress-rv32i.s and  b/test/MC/RISCV/compress-rv64i.s into rv32-compress.s and rv32c-only?
What about compressed instructions that are 64bit only, like C.ADDW. where should I add the tests for these?

> - 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).

How do you suggest we do that, add ll tests and some C tests that include Inline asm?


https://reviews.llvm.org/D42780