[RISCV] Implement MC relaxations for compressed instructions.

[sabuasal]

Summary

This patch implements relaxation for RISCV in the MC layer.
 The following relaxations are currently handled:
 1) Relax C_BEQZ to BEQ and C_BNEZ to BNEZ in RISCV.
 2) Relax and C_J $imm  to JAL x0, $imm  and CJAL to JAL ra, $imm.

[asb]

Is it possible to add tests for this?

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

118

It would be clearer to just have the comment summarise the transform. e.g.:
// c.beqz $rs1, $imm -> beq $rs1, x0, $imm

And similar for other cases

153–158

Surely these (unsigned) casts are unnecessary?

[sabuasal]

Address comments from Alex!

[sabuasal]

Address comments from Alex.

[sabuasal]

Address comments from @asb .

[asb]

Thanks for adding the test case, I added a few minor comments there.

After resolving those, the only remaining issue would be the lack of test coverage for negative offsets.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

95–96

Are you happy to remove this FIXME? Looking at the test case, it seems to me that the correct offset is generated. Please take a look at the objdump and check you agree.

test/MC/RISCV/relaxation.s

1–2 ↗

(On Diff #136459)

Using pipes here is a more user-friendly, as in the case of a test failure you get a nicer command to copy and paste that doesn't produce unwanted temporaries. Given we expect this to work for riscv64 as well, add a RUN line for that too:

# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c < %s \
# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s

5 ↗

(On Diff #136459)

I think checking the instruction location is unnecessary (and a little brittle) as you're already using #INSTR-NEXT. You should include all instruction operands in the check line as that demonstrates that the fixup has been resolved to a sensible value.

e.g.
#INSTR: c.bnez a0, 56

[sabuasal]

test/MC/RISCV/relaxation.s

1–2 ↗

(On Diff #136459)

c.jal is an RV32C instruction only, so I think I have to break this test into two tests. One for 32 and one for 64 bits.

5 ↗

(On Diff #136459)

I added the location because I wanted to check for the instruction size (2 or 4). But I think you are right, it is enough to check the mnemonic for that and check all the operands.

[asb]

Looks good to me. You might want to use c.nop in the tests rather than nop to avoid test churn (changing offsets) once automatic compression is merged.

[sabuasal]

Thanks for the timely review, Alex!

In D43055#1025496, @asb wrote:

Looks good to me. You might want to use c.nop in the tests rather than nop to avoid test churn (changing offsets) once automatic compression is merged.

Updated the nop to c.nop in the committed version.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                   const MCValue &Target, MutableArrayRef<char> Data,
                   uint64_t Value, bool IsResolved) const override;
 
   std::unique_ptr<MCObjectWriter>
   createObjectWriter(raw_pwrite_stream &OS) const override;
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
-                            const MCAsmLayout &Layout) const override {
-    return false;
-  }
+                            const MCAsmLayout &Layout) const override;
 
   unsigned getNumFixupKinds() const override {
     return RISCV::NumTargetFixupKinds;
   }
 
   const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override {
     const static MCFixupKindInfo Infos[RISCV::NumTargetFixupKinds] = {
       // This table *must* be in the order that the fixup_* kinds are defined in
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override {
     if (Kind < FirstTargetFixupKind)
       return MCAsmBackend::getFixupKindInfo(Kind);
 
     assert(unsigned(Kind - FirstTargetFixupKind) < getNumFixupKinds() &&
            "Invalid kind!");
     return Infos[Kind - FirstTargetFixupKind];
   }
 
-  bool mayNeedRelaxation(const MCInst &Inst) const override { return false; }
+  bool mayNeedRelaxation(const MCInst &Inst) const override;
+  unsigned getRelaxedOpcode(unsigned Op) const;
 
   void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {
+                        MCInst &Res) const override;
 
-    report_fatal_error("RISCVAsmBackend::relaxInstruction() unimplemented");
-  }
 
   bool writeNopData(uint64_t Count, MCObjectWriter *OW) const override;
 };
 
+
+bool RISCVAsmBackend::fixupNeedsRelaxation(const MCFixup &Fixup,
+                                           uint64_t Value,
+                                           const MCRelaxableFragment *DF,
+                                           const MCAsmLayout &Layout) const {
+  int64_t Offset = int64_t(Value);
+  switch ((unsigned)Fixup.getKind()) {
+  default:
+    return false;
+  case RISCV::fixup_riscv_rvc_branch:
+    // For compressed branch instructions the immediate must be
+    // in the range [-256, 254].
+    return Offset > 254 || Offset < -256;
+  case RISCV::fixup_riscv_rvc_jump:
+    // For compressed jump instructions the immediate must be
+    // in the range [-2048, 2046].
+    return Offset > 2046 || Offset < -2048;
+  }
+}
+
+void RISCVAsmBackend::relaxInstruction(const MCInst &Inst,
+                                       const MCSubtargetInfo &STI,
+                                       MCInst &Res) const {
+  // TODO: replace this with call to auto generated uncompressinstr() function.
+  switch (Inst.getOpcode()) {
+  default:
+    llvm_unreachable("Opcode not expected!");
+  case RISCV::C_BEQZ:
+    // beq    $rs1, $rs2, $imm12.
+    Res.setOpcode(RISCV::BEQ);
+    Res.addOperand(Inst.getOperand(0));
+    Res.addOperand(MCOperand::createReg(RISCV::X0));
+    Res.addOperand(Inst.getOperand(1));
+    break;
+  case RISCV::C_BNEZ:
+    // bne    $rs1, $rs2, $imm12.
+    Res.setOpcode(RISCV::BNE);
+    Res.addOperand(Inst.getOperand(0));
+    Res.addOperand(MCOperand::createReg(RISCV::X0));
+    Res.addOperand(Inst.getOperand(1));
+    break;
+  case RISCV::C_J:
+    // jal    $rd, $imm20.
+    Res.setOpcode(RISCV::JAL);
+    Res.addOperand(MCOperand::createReg(RISCV::X0));
+    Res.addOperand(Inst.getOperand(0));
+    break;
+  case RISCV::C_JAL:
+    // jal    $rd, $imm20
+    Res.setOpcode(RISCV::JAL);
+    Res.addOperand(MCOperand::createReg(RISCV::X1));
+    Res.addOperand(Inst.getOperand(0));
+    break;
+  }
+}
+
+// Given a compressed control flow instruction this function returns
+// the expanded instruction.
+unsigned RISCVAsmBackend::getRelaxedOpcode(unsigned Op) const {
+  switch (Op) {
+  default:
+    return Op;
+  case RISCV::C_BEQZ:
+    return (unsigned)RISCV::BEQ;
+  case RISCV::C_BNEZ:
+    return (unsigned)RISCV::BNE;
+  case RISCV::C_J:
+  case RISCV::C_JAL: // fall through.
+    return (unsigned)RISCV::JAL;
+  }
+}
+
+bool RISCVAsmBackend::mayNeedRelaxation(const MCInst &Inst) const {
+  return getRelaxedOpcode(Inst.getOpcode()) != Inst.getOpcode();
+}
+
 bool RISCVAsmBackend::writeNopData(uint64_t Count, MCObjectWriter *OW) const {
   bool HasStdExtC = STI.getFeatureBits()[RISCV::FeatureStdExtC];
   unsigned MinNopLen = HasStdExtC ? 2 : 4;
 
   if ((Count % MinNopLen) != 0)
     return false;
 
   // The canonical nop on RISC-V is addi x0, x0, 0.