[MC][Bugfix] Remove redundant parameter for relaxInstruction

Summary:
Before this patch, `relaxInstruction` takes three arguments, the first
argument refers to the instruction before relaxation and the third
argument is the output instruction after relaxation. There are two quite
strange things:
  1) The first argument's type is `const MCInst &`, the third
  argument's type is `MCInst &`, but they may be aliased to the same
  variable
  2) The backends of ARM, AMDGPU, RISC-V, Hexagon assume that the third
  argument is a fresh uninitialized `MCInst` even if `relaxInstruction`
  may be called like `relaxInstruction(Relaxed, STI, Relaxed)` in a
  loop.

In this patch, we drop the thrid argument, and let `relaxInstruction`
directly modify the given instruction. Also, this patch fixes the bug https://bugs.llvm.org/show_bug.cgi?id=45580, which is introduced by D77851, and
breaks the assumption of ARM, AMDGPU, RISC-V, Hexagon.

Reviewers: Razer6, MaskRay, jyknight, asb, luismarques, enderby, rtaylor, colinl, bcain

Reviewed By: Razer6, MaskRay, bcain

Subscribers: bcain, nickdesaulniers, nathanchance, wuzish, annita.zhang, arsenm, dschuff, jyknight, dylanmckay, sdardis, nemanjai, jvesely, nhaehnle, tpr, sbc100, jgravelle-google, kristof.beyls, hiraditya, aheejin, kbarton, fedor.sergeev, asb, rbar, johnrusso, simoncook, sabuasal, niosHD, jrtc27, MaskRay, zzheng, edward-jones, atanasyan, rogfer01, MartinMosbeck, brucehoult, the_o, PkmX, jocewei, Jim, lenary, s.egerton, pzheng, sameer.abuasal, apazos, luismarques, kerbowa, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D78364

include/llvm/MC/MCAsmBackend.h

@@ -161,12 +161,11 @@ public:
 
   /// Relax the instruction in the given fragment to the next wider instruction.
   ///
-  /// \param Inst The instruction to relax, which may be the same as the
+  /// \param [out] Inst The instruction to relax, which is also the relaxed
-  /// output.
+  /// instruction.
   /// \param STI the subtarget information for the associated instruction.
-  /// \param [out] Res On return, the relaxed instruction.
+  virtual void relaxInstruction(MCInst &Inst,
-  virtual void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+                                const MCSubtargetInfo &STI) const {};
-                                MCInst &Res) const = 0;
 
   /// @}
 

lib/MC/MCAssembler.cpp

@@ -926,8 +926,8 @@ bool MCAssembler::relaxInstruction(MCAsmLayout &Layout,
 
   // Relax the fragment.
 
-  MCInst Relaxed;
+  MCInst Relaxed = F.getInst();
-  getBackend().relaxInstruction(F.getInst(), *F.getSubtargetInfo(), Relaxed);
+  getBackend().relaxInstruction(Relaxed, *F.getSubtargetInfo());
 
   // Encode the new instruction.
   //

lib/MC/MCObjectStreamer.cpp

@@ -408,7 +408,7 @@ void MCObjectStreamer::emitInstructionImpl(const MCInst &Inst,
       (Assembler.isBundlingEnabled() && Sec->isBundleLocked())) {
     MCInst Relaxed = Inst;
     while (getAssembler().getBackend().mayNeedRelaxation(Relaxed, STI))
-      getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed);
+      getAssembler().getBackend().relaxInstruction(Relaxed, STI);
     emitInstToData(Relaxed, STI);
     return;
   }

lib/MCA/CodeEmitter.cpp

@@ -25,7 +25,7 @@ CodeEmitter::getOrCreateEncodingInfo(unsigned MCID) {
   const MCInst &Inst = Sequence[MCID];
   MCInst Relaxed(Sequence[MCID]);
   if (MAB.mayNeedRelaxation(Inst, STI))
-    MAB.relaxInstruction(Inst, STI, Relaxed);
+    MAB.relaxInstruction(Relaxed, STI);
 
   EI.first = Code.size();
   MCE.encodeInstruction(Relaxed, VecOS, Fixups, STI);

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

@@ -93,8 +93,8 @@ public:
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override;
+                        const MCSubtargetInfo &STI) const override;
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 
   void HandleAssemblerFlag(MCAssemblerFlag Flag) {}
@@ -467,9 +467,8 @@ bool AArch64AsmBackend::fixupNeedsRelaxation(const MCFixup &Fixup,
   return int64_t(Value) != int64_t(int8_t(Value));
 }
 
-void AArch64AsmBackend::relaxInstruction(const MCInst &Inst,
+void AArch64AsmBackend::relaxInstruction(MCInst &Inst,
-                                         const MCSubtargetInfo &STI,
+                                         const MCSubtargetInfo &STI) const {
-                                         MCInst &Res) const {
   llvm_unreachable("AArch64AsmBackend::relaxInstruction() unimplemented");
 }
 

lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp

@@ -40,8 +40,8 @@ public:
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override;
+                        const MCSubtargetInfo &STI) const override;
 
   bool mayNeedRelaxation(const MCInst &Inst,
                          const MCSubtargetInfo &STI) const override;
@@ -54,12 +54,13 @@ public:
 
 } //End anonymous namespace
 
-void AMDGPUAsmBackend::relaxInstruction(const MCInst &Inst,
+void AMDGPUAsmBackend::relaxInstruction(MCInst &Inst,
-                                        const MCSubtargetInfo &STI,
+                                        const MCSubtargetInfo &STI) const {
-                                        MCInst &Res) const {
+  MCInst Res;
   unsigned RelaxedOpcode = AMDGPU::getSOPPWithRelaxation(Inst.getOpcode());
   Res.setOpcode(RelaxedOpcode);
   Res.addOperand(Inst.getOperand(0));
+  Inst = std::move(Res);
   return;
 }
 

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

@@ -328,9 +328,8 @@ bool ARMAsmBackend::fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
   return reasonForFixupRelaxation(Fixup, Value);
 }
 
-void ARMAsmBackend::relaxInstruction(const MCInst &Inst,
+void ARMAsmBackend::relaxInstruction(MCInst &Inst,
-                                     const MCSubtargetInfo &STI,
+                                     const MCSubtargetInfo &STI) const {
-                                     MCInst &Res) const {
   unsigned RelaxedOp = getRelaxedOpcode(Inst.getOpcode(), STI);
 
   // Sanity check w/ diagnostic if we get here w/ a bogus instruction.
@@ -346,17 +345,18 @@ void ARMAsmBackend::relaxInstruction(const MCInst &Inst,
   // have to change the operands too.
   if ((Inst.getOpcode() == ARM::tCBZ || Inst.getOpcode() == ARM::tCBNZ) &&
       RelaxedOp == ARM::tHINT) {
+    MCInst Res;
     Res.setOpcode(RelaxedOp);
     Res.addOperand(MCOperand::createImm(0));
     Res.addOperand(MCOperand::createImm(14));
     Res.addOperand(MCOperand::createReg(0));
+    Inst = std::move(Res);
     return;
   }
 
   // The rest of instructions we're relaxing have the same operands.
   // We just need to update to the proper opcode.
-  Res = Inst;
+  Inst.setOpcode(RelaxedOp);
-  Res.setOpcode(RelaxedOp);
 }
 
 bool ARMAsmBackend::writeNopData(raw_ostream &OS, uint64_t Count) const {

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.h

@@ -66,8 +66,8 @@ public:
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override;
+                        const MCSubtargetInfo &STI) const override;
 
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 

lib/Target/AVR/MCTargetDesc/AVRAsmBackend.h

@@ -62,9 +62,6 @@ public:
     return false;
   }
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {}
-
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,

lib/Target/BPF/MCTargetDesc/BPFAsmBackend.cpp

@@ -48,9 +48,6 @@ public:
     return false;
   }
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {}
-
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 };
 

lib/Target/Hexagon/MCTargetDesc/HexagonAsmBackend.cpp

@@ -651,11 +651,12 @@ public:
     llvm_unreachable("Handled by fixupNeedsRelaxationAdvanced");
   }
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override {
+                        const MCSubtargetInfo &STI) const override {
     assert(HexagonMCInstrInfo::isBundle(Inst) &&
            "Hexagon relaxInstruction only works on bundles");
 
+    MCInst Res;
     Res.setOpcode(Hexagon::BUNDLE);
     Res.addOperand(MCOperand::createImm(Inst.getOperand(0).getImm()));
     // Copy the results into the bundle.
@@ -679,6 +680,8 @@ public:
       // now copy over the original instruction(the one we may have extended)
       Res.addOperand(MCOperand::createInst(I.getInst()));
     }
+
+    Inst = std::move(Res);
     (void)Update;
     assert(Update && "Didn't find relaxation target");
   }

lib/Target/Lanai/MCTargetDesc/LanaiAsmBackend.cpp

@@ -74,10 +74,6 @@ public:
     return false;
   }
 
-  void relaxInstruction(const MCInst & /*Inst*/,
-                        const MCSubtargetInfo & /*STI*/,
-                        MCInst & /*Res*/) const override {}
-
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 };
 

lib/Target/MSP430/MCTargetDesc/MSP430AsmBackend.cpp

@@ -95,9 +95,6 @@ public:
     return false;
   }
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {}
-
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 };
 

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.h

@@ -74,17 +74,6 @@ public:
     return false;
   }
 
-  /// RelaxInstruction - Relax the instruction in the given fragment
-  /// to the next wider instruction.
-  ///
-  /// \param Inst - The instruction to relax, which may be the same
-  /// as the output.
-  /// \param [out] Res On return, the relaxed instruction.
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {}
-
-  /// @}
-
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,

lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

@@ -192,8 +192,8 @@ public:
     llvm_unreachable("relaxInstruction() unimplemented");
   }
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override {
+                        const MCSubtargetInfo &STI) const override {
     // FIXME.
     llvm_unreachable("relaxInstruction() unimplemented");
   }

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

@@ -139,10 +139,10 @@ bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
   }
 }
 
-void RISCVAsmBackend::relaxInstruction(const MCInst &Inst,
+void RISCVAsmBackend::relaxInstruction(MCInst &Inst,
-                                       const MCSubtargetInfo &STI,
+                                       const MCSubtargetInfo &STI) const {
-                                       MCInst &Res) const {
   // TODO: replace this with call to auto generated uncompressinstr() function.
+  MCInst Res;
   switch (Inst.getOpcode()) {
   default:
     llvm_unreachable("Opcode not expected!");
@@ -173,6 +173,7 @@ void RISCVAsmBackend::relaxInstruction(const MCInst &Inst,
     Res.addOperand(Inst.getOperand(0));
     break;
   }
+  Inst = std::move(Res);
 }
 
 // Given a compressed control flow instruction this function returns

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

@@ -105,9 +105,8 @@ public:
                          const MCSubtargetInfo &STI) const override;
   unsigned getRelaxedOpcode(unsigned Op) const;
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override;
+                        const MCSubtargetInfo &STI) const override;
-
 
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 

lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

@@ -271,8 +271,8 @@ namespace {
       llvm_unreachable("fixupNeedsRelaxation() unimplemented");
       return false;
     }
-    void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+    void relaxInstruction(MCInst &Inst,
-                          MCInst &Res) const override {
+                          const MCSubtargetInfo &STI) const override {
       // FIXME.
       llvm_unreachable("relaxInstruction() unimplemented");
     }

lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp

@@ -63,10 +63,6 @@ public:
                             const MCAsmLayout &Layout) const override {
     return false;
   }
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {
-    llvm_unreachable("SystemZ does do not have assembler relaxation");
-  }
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override {

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp

@@ -64,9 +64,6 @@ public:
     return false;
   }
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
-                        MCInst &Res) const override {}
-
   bool writeNopData(raw_ostream &OS, uint64_t Count) const override;
 };
 

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

@@ -192,8 +192,8 @@ public:
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
 
-  void relaxInstruction(const MCInst &Inst, const MCSubtargetInfo &STI,
+  void relaxInstruction(MCInst &Inst,
-                        MCInst &Res) const override;
+                        const MCSubtargetInfo &STI) const override;
 
   bool padInstructionViaRelaxation(MCRelaxableFragment &RF,
                                    MCCodeEmitter &Emitter,
@@ -825,9 +825,8 @@ bool X86AsmBackend::fixupNeedsRelaxation(const MCFixup &Fixup,
 
 // FIXME: Can tblgen help at all here to verify there aren't other instructions
 // we can relax?
-void X86AsmBackend::relaxInstruction(const MCInst &Inst,
+void X86AsmBackend::relaxInstruction(MCInst &Inst,
-                                     const MCSubtargetInfo &STI,
+                                     const MCSubtargetInfo &STI) const {
-                                     MCInst &Res) const {
   // The only relaxations X86 does is from a 1byte pcrel to a 4byte pcrel.
   bool Is16BitMode = STI.getFeatureBits()[X86::Mode16Bit];
   unsigned RelaxedOp = getRelaxedOpcode(Inst, Is16BitMode);
@@ -840,8 +839,7 @@ void X86AsmBackend::relaxInstruction(const MCInst &Inst,
     report_fatal_error("unexpected instruction to relax: " + OS.str());
   }
 
-  Res = Inst;
+  Inst.setOpcode(RelaxedOp);
-  Res.setOpcode(RelaxedOp);
 }
 
 /// Return true if this instruction has been fully relaxed into it's most
@@ -915,8 +913,8 @@ bool X86AsmBackend::padInstructionViaRelaxation(MCRelaxableFragment &RF,
     // encoding size without impacting performance.
     return false;
 
-  MCInst Relaxed;
+  MCInst Relaxed = RF.getInst();
-  relaxInstruction(RF.getInst(), *RF.getSubtargetInfo(), Relaxed);
+  relaxInstruction(Relaxed, *RF.getSubtargetInfo());
 
   SmallVector<MCFixup, 4> Fixups;
   SmallString<15> Code;

test/MC/RISCV/rv64-relax-all.s

@@ -0,0 +1,15 @@
+# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c %s | llvm-objdump -d -M no-aliases --no-show-raw-insn - | FileCheck %s --check-prefix=INSTR
+
+# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c %s --mc-relax-all | llvm-objdump -d -M no-aliases --no-show-raw-insn - | FileCheck %s --check-prefix=RELAX-INSTR
+
+## Check the instructions are relaxed correctly
+
+NEAR:
+
+# INSTR:           c.beqz    a0, 0 <NEAR>
+# RELAX-INSTR:     beq    a0, zero, 0 <NEAR>
+c.beqz a0, NEAR
+
+# INSTR:           c.j    -2 <NEAR>
+# RELAX-INSTR:     jal    zero, -4 <NEAR>
+c.j NEAR