This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] CompressPat Tablegen-driven Instruction Compression
ClosedPublic

Authored by sabuasal on Jan 31 2018, 7:50 PM.

Details

Summary

This patch implements a tablegen-driven Instruction Compression
mechanisms for generating RISCV compressed
instructions (C Extension) from the expanded instruction form.

This tablegen backend processes CompressPat declarations in a
td file and generates all the compile-time and runtime checks
required to validate the declarations, validate the input
operands and generate correct instuctions.
The checks include validating register operands, immediate
operands, fixed register operands and fixed immediate operands.

Example:
class CompressPat<dag input, dag output> {
  dag Input  = input;
  dag Output    = output;
  list<Predicate> Predicates = [];
 }

let Predicates = [HasStdExtC] in {
def : CompressPat<(ADD GPRNoX0:$rs1, GPRNoX0:$rs1, GPRNoX0:$rs2),
                   (C_ADD GPRNoX0:$rs1, GPRNoX0:$rs2)>;
}

The result is an auto-generated header file
'RISCVGenCompressEmitter.inc' which exports two functions for
compressing/uncompressing MCInst instructions, plus
some helper functions:

bool compressInst(MCInst& OutInst, const MCInst &MI,
                    const MCSubtargetInfo &STI,
                    MCContext &Context);

bool uncompressInst(MCInst& OutInst, const MCInst &MI,
                      const MCRegisterInfo &MRI,
                      const MCSubtargetInfo &STI);

Diff Detail

Event Timeline

sabuasal created this revision.Jan 31 2018, 7:50 PM
sabuasal retitled this revision from [RISCV] Tablegen-driven Instruction Compression to [WIP] [RISCV] CompressPat Tablegen-driven Instruction Compression.Feb 1 2018, 5:40 PM
sabuasal updated this revision to Diff 132531.Feb 1 2018, 7:15 PM

updates patch. rbased on top of https://reviews.llvm.org/D42782.
minor formatting.

sabuasal updated this revision to Diff 132722.Feb 2 2018, 7:29 PM

Comment cleanup.
removed unneeded checks for register class if register operand is tied.

sabuasal updated this revision to Diff 134771.Feb 16 2018, 4:42 PM
sabuasal retitled this revision from [WIP] [RISCV] CompressPat Tablegen-driven Instruction Compression to [RISCV] CompressPat Tablegen-driven Instruction Compression.
sabuasal edited the summary of this revision. (Show Details)Feb 26 2018, 3:52 PM
sabuasal edited the summary of this revision. (Show Details)
sabuasal updated this revision to Diff 136401.Feb 28 2018, 2:28 PM

Re-based r326138.
removed dead code.

asb added a comment.Mar 1 2018, 3:42 AM

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).
lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
16 ↗(On Diff #136401)

RISCVELFStreamer.h should go first, as it is the counterpart to RISCVELFStreamer.cpp ('main module header')

lib/Target/RISCV/RISCVInstrInfoC.td
533

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.

asb added a comment.Mar 15 2018, 7:21 AM

By the way, I'm getting the following error while trying to build this:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "LLVMRISCVAsmPrinter" of type SHARED_LIBRARY
    depends on "LLVMRISCVDesc" (weak)
  "LLVMRISCVDesc" of type SHARED_LIBRARY
    depends on "LLVMRISCVAsmPrinter" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

If you're not seeing this, it may be because I build with -DBUILD_SHARED_LIBS=true during development.

asb added a comment.Mar 15 2018, 10:12 AM
In D42780#1038674, @asb wrote:

By the way, I'm getting the following error while trying to build this:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "LLVMRISCVAsmPrinter" of type SHARED_LIBRARY
    depends on "LLVMRISCVDesc" (weak)
  "LLVMRISCVDesc" of type SHARED_LIBRARY
    depends on "LLVMRISCVAsmPrinter" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

If you're not seeing this, it may be because I build with -DBUILD_SHARED_LIBS=true during development.

I think the best solution is to avoid the layering issue altogether and move evaluateMCOpAsConstantImm and evaluateMCOpAsSymbolRef to MCInst.cpp as methods of MCOperand. I got this compiling after making that adjustment, renaming to evaluateAsConstantImm and evaluateAsSymbolRef. However I note that evaluateAsSymbolRef isn't really an accurate name, is this seems to be a simple predicate that doesn't return any data.

Looking at RISCVGenCompressInstEmitter:

  • I'm not sure I see the need for the RISCVAreEqualOperands helper. You already don't consistently check that every operand isReg. I think it's fine to rely on invalid operands at this stage being a programming error which would be picked up in an asserts build by getReg presumably.
  • // caseFOO should be // case FOO

The tests in compress-rv*.s would be more consistent with other similar instruction tests in test/MC/RISCV (and test/MC/{X86,Sparc} and others) if the CHECK lines came before the instruction. Putting them after is a reasonable choice and I can see that test/MC/AArch64 seems to prefer this, but it would be better to conform to the other RISCV MC instruction tests.

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

sabuasal marked 2 inline comments as done.Mar 27 2018, 8:17 PM
In D42780#1039073, @asb wrote:
In D42780#1038674, @asb wrote:

By the way, I'm getting the following error while trying to build this:

CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "LLVMRISCVAsmPrinter" of type SHARED_LIBRARY
    depends on "LLVMRISCVDesc" (weak)
  "LLVMRISCVDesc" of type SHARED_LIBRARY
    depends on "LLVMRISCVAsmPrinter" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.

If you're not seeing this, it may be because I build with -DBUILD_SHARED_LIBS=true during development.

I think the best solution is to avoid the layering issue altogether and move evaluateMCOpAsConstantImm and evaluateMCOpAsSymbolRef to MCInst.cpp as methods of MCOperand. I got this compiling after making that adjustment, renaming to evaluateAsConstantImm and evaluateAsSymbolRef. However I note that evaluateAsSymbolRef isn't really an accurate name, is this seems to be a simple predicate that doesn't return any data.

fixed.

Looking at RISCVGenCompressInstEmitter:

  • I'm not sure I see the need for the RISCVAreEqualOperands helper. You already don't consistently check that every operand isReg. I think it's fine to rely on invalid operands at this stage being a programming error which would be picked up in an asserts build by getReg presumably.

This function is added to check for tied operands, weather it being a register or immediate. for example , if you use the patters (INSTR $rs $rs ) we need a check that these two operands are equal in value (if resisters, hold the same register, if immediates hold the same immediate) . Currently RISCV doesn't have timed immediate operands but this backend is written keeping in mind that new patterns or different ISAs might use it.

so in case Immediate check is need this function should be updated to:
return ( MCOp1.isReg() && MCOp2.isReg() &&

   (MCOp1.getReg() == MCOp2.getReg())  ) || 
( MCOp1.isImm() && MCOp2.isImm() &&
   (MCOp1.getImm() == MCOp2.getImm())

What places do you think we should have checked for an operand being a register? the combination of validation time (compile the td file) and runtime check should catch all cases I think. Maybe we are missing something?

  • // caseFOO should be // case FOO

Nice catch :), fixed!

The tests in compress-rv*.s would be more consistent with other similar instruction tests in test/MC/RISCV (and test/MC/{X86,Sparc} and others) if the CHECK lines came before the instruction. Putting them after is a reasonable choice and I can see that test/MC/AArch64 seems to prefer this, but it would be better to conform to the other RISCV MC instruction tests.

Fixed.

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
16 ↗(On Diff #136401)

Removed the change.
This was done be clang-format by the way, possible bug?

lib/Target/RISCV/RISCVInstrInfoC.td
533

updated the comment.

Added C_NOP compression (ADDI X0 X0 0) ----> C_NOP

Add EBREAK ---> C_EBREAK.

sabuasal marked 3 inline comments as done.Mar 27 2018, 8:21 PM
sabuasal updated this revision to Diff 140035.Mar 27 2018, 8:25 PM
  • addressed comments by @asb
asb added a comment.Apr 4 2018, 8:24 AM
In 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 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.

asb added a comment.Apr 4 2018, 8:47 AM
In D42780#1039073, @asb wrote:

Looking at RISCVGenCompressInstEmitter:

  • I'm not sure I see the need for the RISCVAreEqualOperands helper. You already don't consistently check that every operand isReg. I think it's fine to rely on invalid operands at this stage being a programming error which would be picked up in an asserts build by getReg presumably.

This function is added to check for tied operands, weather it being a register or immediate. for example , if you use the patters (INSTR $rs $rs ) we need a check that these two operands are equal in value (if resisters, hold the same register, if immediates hold the same immediate) . Currently RISCV doesn't have timed immediate operands but this backend is written keeping in mind that new patterns or different ISAs might use it.

so in case Immediate check is need this function should be updated to:
return ( MCOp1.isReg() && MCOp2.isReg() &&

   (MCOp1.getReg() == MCOp2.getReg())  ) || 
( MCOp1.isImm() && MCOp2.isImm() &&
   (MCOp1.getImm() == MCOp2.getImm())

What places do you think we should have checked for an operand being a register? the combination of validation time (compile the td file) and runtime check should catch all cases I think. Maybe we are missing something?

Most of the logic other than RISCVAreEqualOperands seems to just use .getReg with no guard. e.g. the generated if statement for add -> c.mv:

if (STI.getFeatureBits()[RISCV::FeatureStdExtC] &&
  (MI.getOperand(1).getReg() == RISCV::X0) &&
  (MRI.getRegClass(RISCV::GPRNoX0RegClassID).contains(MI.getOperand(0).getReg())) &&
  (MRI.getRegClass(RISCV::GPRNoX0RegClassID).contains(MI.getOperand(2).getReg()))) {

Really I was suggesting that the helper seems unnecessary, and you might as well just directly emit MCOp1.getReg() == MCOp2.getReg() into the if statement.

sabuasal updated this revision to Diff 141103.Apr 4 2018, 8:39 PM
  • Removed the function "AreEqualOpernds"
  • ----added sanity test cases--- compression run lines to tests:

    test/CodeGen/RISCV/branch.ll test/CodeGen/RISCV/alu32.ll

    added test: CodeGen/RISCV/compress-inline-asm.ll To check for compressing in line assembly.
asb added a comment.Apr 5 2018, 1:58 AM

Thanks for the test changes.

I think this is now looking good to land if my minor comments are addressed. As with any piece of code there may be further cleanups or improvements to make in the future, but they are probably best tackled once in-tree.

Do you think some of the uses of assert and llvm_unreachable in the tablegen emitter might be better handled as a fatal error to ensure we error out in non-debug builds? It's more developer friendly that way, and it's not uncommon for people to use -DLLVM_OPTIMIZED_TABLEGEN=True to improve build times, meaning that even in your debug-enabled LLVM build you get a non-debug Tablegen.

From your perspective, is there any aspect in this patch that you'd like people to take a closer look at?

include/llvm/MC/MCInst.h
117

Newline after this?

lib/MC/MCInst.cpp
47

Would this function be better named as isSymbolRef or isBareSymbolRef or something that reflects the fact it simply returns true if we have a SymbolRef with a MCSymbolRefExpr::VK_None kind.

test/CodeGen/RISCV/compress-inline-asm.ll
21–26

I don't think we need this extraneous stuff from clang. I'd just keep this as a truly minimal test case, like one of the example in test/CodeGen/RISCV/inline-asm.ll.

sabuasal updated this revision to Diff 141245.Apr 5 2018, 5:27 PM
sabuasal marked 3 inline comments as done.
  • Renamed evaluateSymbolRef to isBaresymbolRef
  • Updated compress-inline test case.
  • Looked over the assert and unreachable statements and converted needed to PrintFatalError.
asb accepted this revision.Apr 6 2018, 2:57 AM

Thanks, this looks good to me now.

This revision is now accepted and ready to land.Apr 6 2018, 2:57 AM