[RISCV] Hooks for enabling instruction compression

[sabuasal]

Summary

This patch inserts calls in RISCV backend to call instruction
compression\uncompression logic.

Calls were inserted in three locations:
1) RISCVAsmParser::MatchAndEmitInstruction:
    Inserted a call to compressInst() to compresses instructions
    parsed by llvm-mc coming from an ASM input.
 2) RISCVAsmPrinter::EmitInstruction:
     Inserted a call to compressInst() to compress instructions coming
     that were lowered from Machine Instructions (MachineInstr).
  3) RVInstPrinter::printInst:
      Inserted a call to uncompressInst() to print the expanded
      version of the instruction instead of the compressed one (e.g,
      add s0, s0, a5 instead of c.add s0, a5) when -riscv-no-aliases
      is not passed.

[asb]

Thanks Ana for outlining your current thinking. Without reviewing the fine details, this seems in the direction we were all discussing.

Although in the current GNU tools control of compression/decompression is intrinsically linked to the -M no-aliases flag, we might want to add some more fine-grained control here (after all, it seems reasonable that I'd want to disable automatic compression but still use pseudoinstructions like mv). Something we can think about later though.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

46

I think the logic should actually be the other way - we uncompress if !NoAliases (i.e. by default if we decode a c.addi we print it as addi)

[apazos]

Rebased - depends on Shiva's https://reviews.llvm.org/D41658

[apazos]

Also depends on the WIP in https://reviews.llvm.org/D42228

[sabuasal]

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

46

Hi Alex,

I think we are actually in agreement. If we have an encoded c.addi and -riscv-noaliases is passed, NoAlias will be true, uncompress will not be called so we print the c.addi.

[efriedma]

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

52

I'm not sure it makes sense to do compression here. Software using the MC layer should have full control over what instructions it's emitting, where possible. And to get the right output with "-riscv-no-aliases", you have to run compression before you output assembly.

If the backend is okay delaying compression until really late, maybe you could call it in LowerRISCVMachineInstrToMCInst() or something like that?

[asb]

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

46

We are now in agreement - the logic was the other way around in the original diff. I don't see a way to mark my comment on someone else's patch as 'done' - but it's fine now.

49–51

I think const_cast would be preferable here. Pehaps we could get rid of Res ? (const MCInst *)&NewMI : MI which makes it harder to read - maybe using a MCInst* that is initialised to MI and updated to &NewMI if necessary.

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

52

It's important that we reuse the same MCInst->MCInst mapping both for the codegen and assembly/disassembly path, but you're right - it seems like it would be viable to call this in LowerRISCVMachineInstrToMCInst - doing the usual lowering and then trying to transform OutMI. Do you see any issues with that Ana/Sameer?

[apazos]

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

53

The instruction will only be transformed if compression extension is enabled.
If the user enables compression, but does not want a particular instruction to be compressed, RISCV has no assembler directive to allow that (does it Alex? Maybe we can request that feature?). As Eli knows ARM supports "adds.w" to allow that kind of fine grain control.
We need to also check if LowerRISCVMachineInstrToMCInst is invoked when coming from assembly/disassembly path.

[sabuasal]

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

52

@efriedma @asb

"LowerRISCVMachineInstrToMCInst" Is not on the path for the Assembler\Disassembler (llvm-mc). Adding compression logic there will result in llvm-mc not compressing instructions passed from an ASM file. The reason compression changes where added to the streamer is because it is the onlt location where all paths (Assembler. MI lowering) meet.

[apazos]

Addressed formatting comments.

[sabuasal]

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

49–51

Done.

[asb]

Adding llvm-commits as a subscriber. A couple more comments inline

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

52

I recognise LowerRISCVMachineInstrToMCInst won't be called on the llvm-mc path, but this patch does add calls to the asm parser and instprinter, which at first glance seems sufficient for the llvm-mc case. Is that not so? It's been a while since I looked at this part of the instruction compression discussion, so I might be missing/forgetting something.

Updating this diff with a patch summary briefly explaining the locations compressInst is called would be helpful.

53

There is no current directive to control automatic compression separately from support for the C extension. If you want to compile an assembly file with support for the C extension but have complete control over when compressed instructions are chosen you currently need to use .option rvc and .option norvc (in gas - not yet implemented in llvm-mc). e.g.

.option norvc
addi x1, x1, 1
.option rvc
c.addi x1, 1

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

53

I do think it would be good to have an option for this. I identified this lack in https://github.com/riscv/riscv-toolchain-conventions#issues-for-consideration-4, and it would probably be worth trying again to engage the GCC devs and RISC-V community in trying to address some of the holes detailed in my toolchain-conventions document.

[apazos]

We need to put in the debugger to double check the flow, I recall this flow:
.s ->.o
AsmParser.cpp ->...-> RISCVAsmParser::MatchAndEmitInstruction-> MatchInstructionImpl-> Out.EmitInstruction…
.o->.s
Disassemble.cpp -> ... ->Streamer.EmitInstruction(Inst, STI);-> InstPrinter->printInst(&Inst, OS, "", STI);-> printInstruction(MI, STI, O) in RISCVInstrPrinter

The goal was to try to compress the instr before emitting the instr. And uncompress the instr when printing, depending on the alias flag.

[sabuasal]

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

52

I added a summary for why every hook was added to the patch.

For the llvm-mc and llvm-objdump path having the hooks in AsmParser and in Instr Printer is sufficient for what we are trying to achieve.

The hook to the Elfstreamer was added for the path on 1.c ---> 1.o. This could be moved earlier like @efriedma suggested (I tried it by calling compression in RISCVAsmPrinter::EmitInstruction right after it calls LowerRISCVMachineInstrToMCInst). This also has another advantage of saving a call to compressInst for the path (1.s ---> 1.o); as is compressInst will be called both at the AsmParster and at the ELfStreamer.

However, the only possible side effect is that we will no longer be matching the gcc fronted in this. going from a (1.c ---> 1.s) gcc never emits a compressed asm instruction. While clang can do that if we pass -riscv-no-aliases. The gcc equivilent flag for this (-M -no-aliases) only works with gcc-objdump and not the fronted.

If we don't forsee this as an issue then we can move the hook with no problem.

[asb]

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

52

Thanks for confirming Sameer. Moving earlier, to e.g. LowerRISCVMachineInstrToMCInst seems preferable.

I think the fact we can have -riscv-no-aliases affect .s emission is an advantage vs GCC.

[sabuasal]

Removed the call to EmitInstr in ELFStream.
Updated test cases.

[asb]

This diff seems to have accidentally picked up changes to a number of tests.

Other than that, this looks good to me. I expect that in the future we'll want to have the option of having the C extension enabled but not performing automatic compression, but that can always be added later.

[sabuasal]

Hi Alex,

Thanks for the review,

Yes, the changes in the test cases are caused by the fact that I rebased on the latest llvm (with mc relaxation enabled).

[sabuasal]

Committed with: https://reviews.llvm.org/D45385

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
 
+// Include the auto-generated portion of the compress emitter.
+#define GEN_COMPRESS_INSTR
+#include "RISCVGenCompressInstEmitter.inc"
+
 namespace {
 struct RISCVOperand;
 
 class RISCVAsmParser : public MCTargetAsmParser {
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
   bool isRV64() const { return getSTI().hasFeature(RISCV::Feature64Bit); }
 
   unsigned validateTargetOperandClass(MCParsedAsmOperand &Op,
bool RISCVAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                              MCStreamer &Out,
                                              uint64_t &ErrorInfo,
                                              bool MatchingInlineAsm) {
   MCInst Inst;
 
   switch (MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm)) {
   default:
     break;
-  case Match_Success:
+  case Match_Success: {
+    MCInst CInst;
+    bool Res = compressInst(CInst, Inst, getSTI(), Out.getContext());
+    CInst.setLoc(IDLoc);
     Inst.setLoc(IDLoc);
-    Out.EmitInstruction(Inst, getSTI());
+    Out.EmitInstruction((Res ? CInst : Inst), getSTI());
     return false;
+  }
   case Match_MissingFeature:
     return Error(IDLoc, "instruction use requires an option to be enabled");
   case Match_MnemonicFail:
     return Error(IDLoc, "unrecognized instruction mnemonic");
   case Match_InvalidOperand: {
     SMLoc ErrorLoc = IDLoc;
     if (ErrorInfo != ~0U) {
       if (ErrorInfo >= Operands.size())

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

 using namespace llvm;
 
 #define DEBUG_TYPE "asm-printer"
 
 // Include the auto-generated portion of the assembly writer.
 #define PRINT_ALIAS_INSTR
 #include "RISCVGenAsmWriter.inc"
 
+// Include the auto-generated portion of the compress emitter.
+#define GEN_UNCOMPRESS_INSTR
+#include "RISCVGenCompressInstEmitter.inc"
+
 static cl::opt<bool>
 NoAliases("riscv-no-aliases",
             cl::desc("Disable the emission of assembler pseudo instructions"),
             cl::init(false),
             cl::Hidden);
 
 void RISCVInstPrinter::printInst(const MCInst *MI, raw_ostream &O,
                                  StringRef Annot, const MCSubtargetInfo &STI) {
-  if (NoAliases || !printAliasInstr(MI, STI, O))
-    printInstruction(MI, STI, O);
+  bool Res = false;
+  MCInst NewMI;
+  if (!NoAliases)
+    Res = uncompressInst(NewMI, *MI, MRI, STI);
+  if (NoAliases ||
+    !printAliasInstr((Res ? (const MCInst *)&NewMI : MI), STI, O))
+    printInstruction((Res ? (const MCInst *)&NewMI : MI), STI, O);
   printAnnotation(O, Annot);
 }
 
 void RISCVInstPrinter::printRegName(raw_ostream &O, unsigned RegNo) const {
   O << getRegisterName(RegNo);
 }
 
 void RISCVInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h

 #ifndef LLVM_LIB_TARGET_RISCV_RISCVELFSTREAMER_H
 #define LLVM_LIB_TARGET_RISCV_RISCVELFSTREAMER_H
 
 #include "RISCVTargetStreamer.h"
 #include "llvm/MC/MCELFStreamer.h"
 
 namespace llvm {
 
+class RISCVELFStreamer;
+
 class RISCVTargetELFStreamer : public RISCVTargetStreamer {
 public:
-  MCELFStreamer &getStreamer();
+  RISCVELFStreamer &getStreamer();
   RISCVTargetELFStreamer(MCStreamer &S, const MCSubtargetInfo &STI);
 };
+
+class RISCVELFStreamer : public MCELFStreamer {
+public:
+  friend class RISCVTargetELFStreamer;
+
+  RISCVELFStreamer(MCContext &Context, std::unique_ptr<MCAsmBackend> TAB,
+                   raw_pwrite_stream &OS,
+                   std::unique_ptr<MCCodeEmitter> Emitter)
+      : MCELFStreamer(Context, std::move(TAB), OS, std::move(Emitter)) {}
+
+  /// This function is used to emit instruction data into the ELF
+  /// streamer. We override it to do instruction compression.
+  void EmitInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+                       bool) override;
+};
+
+MCELFStreamer *createRISCVELFStreamer(MCContext &Context,
+                                      std::unique_ptr<MCAsmBackend> TAB,
+                                      raw_pwrite_stream &OS,
+                                      std::unique_ptr<MCCodeEmitter> Emitter,
+                                      bool RelaxAll);
 }
 #endif

lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

 //===-- RISCVELFStreamer.cpp - RISCV ELF Target Streamer Methods ----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides RISCV specific target streamer methods.
 //
 //===----------------------------------------------------------------------===//
 
 #include "RISCVMCTargetDesc.h"
 #include "RISCVELFStreamer.h"
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 
 using namespace llvm;
 
+// Include the auto-generated portion of the compress emitter.
+#define GEN_COMPRESS_INSTR
+#include "RISCVGenCompressInstEmitter.inc"
+
 // This part is for ELF object output.
 RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
                                                const MCSubtargetInfo &STI)
     : RISCVTargetStreamer(S) {
   MCAssembler &MCA = getStreamer().getAssembler();
 
   const FeatureBitset &Features = STI.getFeatureBits();
 
   unsigned EFlags = MCA.getELFHeaderEFlags();
 
   if (Features[RISCV::FeatureStdExtC])
     EFlags |= ELF::EF_RISCV_RVC;
 
   MCA.setELFHeaderEFlags(EFlags);
 }
 
-MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
-  return static_cast<MCELFStreamer &>(Streamer);
+RISCVELFStreamer &RISCVTargetELFStreamer::getStreamer() {
+  return static_cast<RISCVELFStreamer &>(Streamer);
+}
+
+void RISCVELFStreamer::EmitInstruction(const MCInst &Inst,
+  const MCSubtargetInfo &STI, bool) {
+  MCInst CInst;
+  bool Res = compressInst(CInst, Inst, STI, getContext());
+  MCELFStreamer::EmitInstruction((Res ? CInst : Inst), STI);
+}
+
+namespace llvm {
+MCELFStreamer *createRISCVELFStreamer(MCContext &Context,
+                                      std::unique_ptr<MCAsmBackend> TAB,
+                                      raw_pwrite_stream &OS,
+                                      std::unique_ptr<MCCodeEmitter> Emitter,
+                                      bool RelaxAll) {
+  RISCVELFStreamer *S =
+    new RISCVELFStreamer(Context, std::move(TAB), OS, std::move(Emitter));
+  return S;
 }
+} // end namespace llvm

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

 //===----------------------------------------------------------------------===//
 
 #include "RISCVMCTargetDesc.h"
 #include "InstPrinter/RISCVInstPrinter.h"
 #include "RISCVMCAsmInfo.h"
 #include "RISCVELFStreamer.h"
 #include "RISCVTargetStreamer.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"
 
 #define GET_INSTRINFO_MC_DESC

 static MCTargetStreamer *
 createRISCVObjectTargetStreamer(MCStreamer &S, const MCSubtargetInfo &STI) {
   const Triple &TT = STI.getTargetTriple();
   if (TT.isOSBinFormatELF())
     return new RISCVTargetELFStreamer(S, STI);
   return new RISCVTargetStreamer(S);
 }
 
+static MCStreamer *createELFStreamer(const Triple &T, MCContext &Ctx,
+                                     std::unique_ptr<MCAsmBackend> &&TAB,
+                                     raw_pwrite_stream &OS,
+                                     std::unique_ptr<MCCodeEmitter> &&Emitter,
+                                     bool RelaxAll) {
+  return createRISCVELFStreamer(Ctx, std::move(TAB), OS, std::move(Emitter),
+                                RelaxAll);
+}
+
 extern "C" void LLVMInitializeRISCVTargetMC() {
   for (Target *T : {&getTheRISCV32Target(), &getTheRISCV64Target()}) {
     TargetRegistry::RegisterMCAsmInfo(*T, createRISCVMCAsmInfo);
     TargetRegistry::RegisterMCInstrInfo(*T, createRISCVMCInstrInfo);
     TargetRegistry::RegisterMCRegInfo(*T, createRISCVMCRegisterInfo);
     TargetRegistry::RegisterMCAsmBackend(*T, createRISCVAsmBackend);
     TargetRegistry::RegisterMCCodeEmitter(*T, createRISCVMCCodeEmitter);
     TargetRegistry::RegisterMCInstPrinter(*T, createRISCVMCInstPrinter);
     TargetRegistry::RegisterMCSubtargetInfo(*T, createRISCVMCSubtargetInfo);
+    TargetRegistry::RegisterELFStreamer(*T, createELFStreamer);
     TargetRegistry::RegisterObjectTargetStreamer(
         *T, createRISCVObjectTargetStreamer);
   }
 }