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

Wed Apr 4 08:25:00 PDT 2018

asb added a comment.

In https://reviews.llvm.org/D42780#1048737, @sabuasal wrote:

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

Definitely noisier, though it wasn't too much of a problem for the instruction definitions. I'd lean towards matching that ordering.

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

In https://reviews.llvm.org/D42780#1048737, @sabuasal wrote:

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

Sure, I was just suggesting that with the way these tests are structured, CHECK-BYTES doesn't give you much additional information. If we know the encoding from InstPrinter is correct, it's not a big leap that the encoding in the final object is also fine.

Not a big deal anyway. If you found CHECK-BYTES useful in debugging or think it's worthwhile, lets leave it.

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

Add at least a 'sanity' .ll test (which produces a somewhat broad range of instructions), and a .ll with inline asm. Testing for these would need to include disassembling the generated instruction in the generated ELF, so unfortuantely these .ll files couldn't use update_llc_test_checks.py.

https://reviews.llvm.org/D42780