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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D42780