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

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal marked 2 inline comments as done.
sabuasal added a comment.

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

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


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.



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:16
 #include "RISCVMCTargetDesc.h"
+#include "RISCVELFStreamer.h"
 #include "llvm/BinaryFormat/ELF.h"
----------------
asb wrote:
> 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 ]]')
Removed the change.
This was done be clang-format by the way, possible bug?


================
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),
----------------
asb wrote:
> 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.
updated the comment.

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

Add EBREAK ---> C_EBREAK.


https://reviews.llvm.org/D42780