[RISCV] Insert NOPs and R_RISCV_ALIGN relocation type for .align directive when linker relaxation enabled

[shiva0217]

Summary

.align N alignment fixed by assembler may be changed when the linker relaxation enabled.
In this case, we have to insert N-2 bytes NOPs and R_RISCV_ALIGN relocation type.
In this way, Linker could detect R_RISCV_ALIGN and fix the alignment by removing
NOPs.

To do this:

1. Add parseDirectiveAlign in RISCVAsmParser to parse .align and .p2align directive and then insert PseudoNOPs instruction.

2. Add expandNOPs in RISCVMCCodeEmitter to expand PseudoNOPs to NOPs with R_RISCV_ALIGN relocation type.

3. Add disableAlignmentPaddingInTextSectiontarget hook in MCAsmBackend to disable generic padding for .align and let PseudoNOPs deal with it.

[sabuasal]

Hi Shiva,

Can you please rebase your patch. It is out of date

[shiva0217]

Rebase the patch to trunk@332828

[shiva0217]

Rename thd target hook to disableAlignmentPaddingInTextSection.

[asb]

Hi Shiva. As far as testing goes, I think we really need to see testing of corner cases around enabling/disabling rvc support in the same file.

Could you comment about the decision to introduce the PseudoNOP instruction? The obvious alternative approach would be to try to customise the code that handles MCAlignFragments, adding new hooks if necessary. Have you looked in any detail at doing it that way?

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1046

What about .balign?

1053

This ignores anything after the first argument to .align. Don't we need to duplicate some of the parsing logic from AsmParser::parseDirectiveAlign? I can certainly see that if I change one of the .p2 align to .p2align 2, 1 then that directive is deleted entirely when testing asm printing with llvm-mc -filetype=asm.

[shiva0217]

In D46630#1109492, @asb wrote:

Hi Shiva. As far as testing goes, I think we really need to see testing of corner cases around enabling/disabling rvc support in the same file.

Could you comment about the decision to introduce the PseudoNOP instruction? The obvious alternative approach would be to try to customise the code that handles MCAlignFragments, adding new hooks if necessary. Have you looked in any detail at doing it that way?

Hi Alex. My first thought is to implement by MCAlignFragment, too. But R_RISCV_ALIGN has to insert to the first NOP. So that linker could know where are the Nop sequences start.
I can't find a way to insert R_RISCV_ALIGN by MCAlignFragment infrastructure. So I insert PseudoNOP which could indicate the first NOP position. Not sure it's the best way to do so.

[asb]

I think that customising the handling of MCAlignFragment is going to be the more correct (with regards to the LLVM MC design and layering) and maintainable way.

I haven't fully stepped through the ordering of function calls when producing an object. Is your concern that the current call to writeNopData from writeFragment comes too late to create the relocation?

[shiva0217]

In D46630#1110712, @asb wrote:

I think that customising the handling of MCAlignFragment is going to be the more correct (with regards to the LLVM MC design and layering) and maintainable way.

I haven't fully stepped through the ordering of function calls when producing an object. Is your concern that the current call to writeNopData from writeFragment comes too late to create the relocation?

　　SMLoc FirstNOPLoc = NOP.getLoc();
　　Fixups.push_back(MCFixup::create(0, TotalBytes, MCFixupKind(RISCV::fixup_riscv_align), FirstNOPLoc));

When creating fixup_riscv_align, we need to indicate the first nop position which is recorded by SMLoc data structure.
I can't find a way to implement it in writeFragment yet.

[shiva0217]

Hi Alex. I find a way to emit fixup_riscv_align without introducing PseudoNOPs as you suggest.
The revision D47755 introducing the new target hooks to calculate the nops we need to insert and insert fixup_riscv_align for alignment directive.
Could you help me to check the new approach is ok or not?
Thanks.

[shiva0217]

There is a new revision D47755 to replace this one.

include/llvm/MC/MCAsmBackend.h

public:
 
   /// Hook to check if a relocation is needed for some target specific reason.
   virtual bool shouldForceRelocation(const MCAssembler &Asm,
                                      const MCFixup &Fixup,
                                      const MCValue &Target) {
     return false;
   }
 
+  /// Hook to disable nop insertion for .align padding.
+  /// RISCV .align need insert extra NOPs and R_RISCV_ALIGN relocation type
+  /// when linker relaxation enabled. So we need to disable generic padding
+  /// and let RISCV PseudoNOPs to deal with it.
+  virtual bool disablePaddingAlignment(void) { return false; }
+
   /// Apply the \p Value for given \p Fixup into the provided data fragment, at
   /// the offset specified by the fixup and following the fixup kind as
   /// appropriate. Errors (such as an out of range fixup value) should be
   /// reported via \p Ctx.
   virtual void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                           const MCValue &Target, MutableArrayRef<char> Data,
                           uint64_t Value, bool IsResolved) const = 0;
 

lib/MC/MCAssembler.cpp

uint64_t MCAssembler::computeFragmentSize(const MCAsmLayout &Layout,
 
   case MCFragment::FT_SymbolId:
     return 4;
 
   case MCFragment::FT_Align: {
     const MCAlignFragment &AF = cast<MCAlignFragment>(F);
     unsigned Offset = Layout.getFragmentOffset(&AF);
     unsigned Size = OffsetToAlignment(Offset, AF.getAlignment());
+
+    if (getBackend().disablePaddingAlignment())
+      return 0;
+
     // If we are padding with nops, force the padding to be larger than the
     // minimum nop size.
     if (Size > 0 && AF.hasEmitNops()) {
       while (Size % getBackend().getMinimumNopSize())
         Size += AF.getAlignment();
     }
     if (Size > AF.getMaxBytesToEmit())
       return 0;

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "MCTargetDesc/RISCVTargetStreamer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #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 {
   SmallVector<FeatureBitset, 4> FeatureBitStack;
 
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
   bool isRV64() const { return getSTI().hasFeature(RISCV::Feature64Bit); }
+  bool enableLinkerRelax() const {
+    return getSTI().hasFeature(RISCV::FeatureRelax);
+  }
 
   RISCVTargetStreamer &getTargetStreamer() {
     MCTargetStreamer &TS = *getParser().getStreamer().getTargetStreamer();
     return static_cast<RISCVTargetStreamer &>(TS);
   }
 
   unsigned validateTargetOperandClass(MCParsedAsmOperand &Op,
                                       unsigned Kind) override;
#include "RISCVGenAsmMatcher.inc"
   OperandMatchResultTy parseRegister(OperandVector &Operands,
                                      bool AllowParens = false);
   OperandMatchResultTy parseMemOpBaseReg(OperandVector &Operands);
   OperandMatchResultTy parseOperandWithModifier(OperandVector &Operands);
 
   bool parseOperand(OperandVector &Operands, bool ForceImmediate);
 
   bool parseDirectiveOption();
+  bool parseDirectiveAlign(SMLoc L);
 
   void setFeatureBits(uint64_t Feature, StringRef FeatureString) {
     if (!(getSTI().getFeatureBits()[Feature])) {
       MCSubtargetInfo &STI = copySTI();
       setAvailableFeatures(
           ComputeAvailableFeatures(STI.ToggleFeature(FeatureString)));
     }
   }
bool RISCVAsmParser::ParseDirective(AsmToken DirectiveID) {
   // This returns false if this function recognizes the directive
   // regardless of whether it is successfully handles or reports an
   // error. Otherwise it returns true to give the generic parser a
   // chance at recognizing it.
   StringRef IDVal = DirectiveID.getString();
 
   if (IDVal == ".option")
     return parseDirectiveOption();
+  // We need to insert R_RISCV_ALIGN relocation type if the linker
+  // relaxation enabled. So then linker could fixup the alignment
+  // after relaxation changed code size.
+  else if (enableLinkerRelax() &&
+           ((IDVal == ".align") || (IDVal == ".p2align")))
+    return parseDirectiveAlign(DirectiveID.getLoc());
+
+  return true;
+}
+
+bool RISCVAsmParser::parseDirectiveAlign(SMLoc L) {
+  StringRef AlignStr = getParser().parseStringToEndOfStatement().trim();
+  unsigned Align, Padding;
+  if (AlignStr.getAsInteger(10, Align))
+    return true;
+
+  unsigned AlignInBytes = 1 << Align;
+  // Always insert AlignInBytes - 2 bytes NOPs because RV32IMA object file
+  // may have chance linking with RV32IMAC object file.
+  // Currently, RISCV ABI not restrict linking RV32IMA with RV32IMAC yet.
+  // If RV32IMA can't link with RV32IMAC, we could padding AlignInBytes - 4
+  // bytes NOPs for RV32IMA.
+  Padding = AlignInBytes - 2;
 
+  if ((Padding == 0) || (Padding > AlignInBytes))
     return true;
+
+  const MCSection *Section = getStreamer().getCurrentSectionOnly();
+  assert(Section && "must have section to emit alignment");
+  if (Section->UseCodeAlign()) {
+    MCInst NOPs = MCInstBuilder(RISCV::PseudoNOPs).addImm(Padding);
+    getStreamer().EmitInstruction(NOPs, getSTI());
+    getStreamer().EmitCodeAlignment(AlignInBytes, 0);
+  } else
+    getStreamer().EmitValueToAlignment(AlignInBytes, 0, 1, 0);
+
+  return false;
 }
 
 bool RISCVAsmParser::parseDirectiveOption() {
   MCAsmParser &Parser = getParser();
   // Get the option token.
   AsmToken Tok = Parser.getTok();
   // At the moment only identifiers are supported.
   if (Tok.isNot(AsmToken::Identifier))

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

public:
 
   // Preserve relocation types if the linker relaxation enabled. So then
   // linker could has information to fixup offsets while relaxation.
   bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
                              const MCValue &Target) override {
     return STI.getFeatureBits()[RISCV::FeatureRelax];
   }
 
+  // We need to insert extra NOPs and R_RISCV_ALIGN relocation type for
+  // .align and .p2align when linker relaxation enabled. So we disable
+  // generic padding for alignment directive and let PseudoNOPs to deal
+  // with it.
+  bool disablePaddingAlignment(void) override {
+    return STI.getFeatureBits()[RISCV::FeatureRelax];
+  }
+
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
 
   unsigned getNumFixupKinds() const override {
     return RISCV::NumTargetFixupKinds;
   }
 
+  Optional<MCFixupKind> getFixupKind(StringRef Name) const override {
+    return StringSwitch<Optional<MCFixupKind>>(Name)
+        .Case("R_RISCV_ALIGN", (MCFixupKind)RISCV::fixup_riscv_align)
+        .Default(MCAsmBackend::getFixupKind(Name));
+  }
+
   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
       // RISCVFixupKinds.h.
       //
       // name                      offset bits  flags
       { "fixup_riscv_hi20",         12,     20,  0 },
       { "fixup_riscv_lo12_i",       20,     12,  0 },

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp

unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
   case RISCV::fixup_riscv_rvc_jump:
     return ELF::R_RISCV_RVC_JUMP;
   case RISCV::fixup_riscv_rvc_branch:
     return ELF::R_RISCV_RVC_BRANCH;
   case RISCV::fixup_riscv_call:
     return ELF::R_RISCV_CALL;
   case RISCV::fixup_riscv_relax:
     return ELF::R_RISCV_RELAX;
+  case RISCV::fixup_riscv_align:
+    return ELF::R_RISCV_ALIGN;
   }
 }
 
 std::unique_ptr<MCObjectWriter>
 llvm::createRISCVELFObjectWriter(raw_pwrite_stream &OS, uint8_t OSABI,
                                  bool Is64Bit) {
   return createELFObjectWriter(
       llvm::make_unique<RISCVELFObjectWriter>(OSABI, Is64Bit), OS,
       /*IsLittleEndian=*/true);
 }

lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h

enum Fixups {
   // compressed branch instruction
   fixup_riscv_rvc_branch,
   // fixup_riscv_call - A fixup representing a call attached to the auipc
   // instruction in a pair composed of adjacent auipc+jalr instructions.
   fixup_riscv_call,
   // fixup_riscv_relax - Used to generate an R_RISCV_RELAX relocation type,
   // which indicates the linker may relax the instruction pair.
   fixup_riscv_relax,
+  // fixup_riscv_align - Used to generate an R_RISCV_ALIGN relocation type,
+  // which indicates the linker should fixup the alignment after linker
+  // relaxation.
+  fixup_riscv_align,
 
   // fixup_riscv_invalid - used as a sentinel and a marker, must be last fixup
   fixup_riscv_invalid,
   NumTargetFixupKinds = fixup_riscv_invalid - FirstTargetFixupKind
 };
 } // end namespace RISCV
 } // end namespace llvm
 
 #endif

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

public:
   void encodeInstruction(const MCInst &MI, raw_ostream &OS,
                          SmallVectorImpl<MCFixup> &Fixups,
                          const MCSubtargetInfo &STI) const override;
 
   void expandFunctionCall(const MCInst &MI, raw_ostream &OS,
                           SmallVectorImpl<MCFixup> &Fixups,
                           const MCSubtargetInfo &STI) const;
 
+  unsigned expandNOPs(const MCInst &MI, raw_ostream &OS,
+                      SmallVectorImpl<MCFixup> &Fixups,
+                      const MCSubtargetInfo &STI) const;
+
   /// TableGen'erated function for getting the binary encoding for an
   /// instruction.
   uint64_t getBinaryCodeForInstr(const MCInst &MI,
                                  SmallVectorImpl<MCFixup> &Fixups,
                                  const MCSubtargetInfo &STI) const;
 
   /// Return binary encoding of operand. If the machine operand requires
   /// relocation, record the relocation and return zero.
void RISCVMCCodeEmitter::expandFunctionCall(const MCInst &MI, raw_ostream &OS,
   support::endian::Writer<support::little>(OS).write(Binary);
 
   // Emit JALR Ra, Ra, 0
   TmpInst = MCInstBuilder(RISCV::JALR).addReg(Ra).addReg(Ra).addImm(0);
   Binary = getBinaryCodeForInstr(TmpInst, Fixups, STI);
   support::endian::Writer<support::little>(OS).write(Binary);
 }
 
+// Expand PseudoNOPs to NOPs with R_RISCV_ALIGN relocation type for
+// .align directive when the linker relaxation enabled.
+unsigned RISCVMCCodeEmitter::expandNOPs(const MCInst &MI, raw_ostream &OS,
+                                        SmallVectorImpl<MCFixup> &Fixups,
+                                        const MCSubtargetInfo &STI) const {
+  bool EnableRelax = STI.getFeatureBits()[RISCV::FeatureRelax];
+  int64_t Count = MI.getOperand(0).getImm();
+  unsigned Zero = RISCV::X0;
+  uint32_t Binary32Bit;
+  uint16_t Binary16Bit;
+  bool isFirstNOP = true;
+  unsigned MCNum = 0;
+  const MCExpr *TotalBytes = MCConstantExpr::create(Count, Ctx);
+  MCInst NOP, C_NOP;
+  SMLoc FirstNOPLoc;
+
+  assert(EnableRelax && "Expected PseudoNOPs when linker relaxation enabled");
+
+  assert(((Count % 2) == 0) && "Expected multiple of two");
+
+  // Emit 32-bit NOP.
+  uint64_t Nop32Count = Count / 4;
+  for (uint64_t i = Nop32Count; i != 0; --i) {
+    NOP = MCInstBuilder(RISCV::ADDI).addReg(Zero).addReg(Zero).addImm(0);
+    Binary32Bit = getBinaryCodeForInstr(NOP, Fixups, STI);
+    support::endian::Writer<support::little>(OS).write(Binary32Bit);
+    ++MCNum;
+
+    if (isFirstNOP) {
+      isFirstNOP = false;
+      FirstNOPLoc = NOP.getLoc();
+    }
+  }
+
+  // Emit 16-bit C_NOP.
+  uint64_t Nop16Count = (Count - Nop32Count * 4) / 2;
+  for (uint64_t i = Nop16Count; i != 0; --i) {
+    C_NOP = MCInstBuilder(RISCV::C_NOP);
+    Binary16Bit = getBinaryCodeForInstr(C_NOP, Fixups, STI);
+    support::endian::Writer<support::little>(OS).write<uint16_t>(Binary16Bit);
+    ++MCNum;
+
+    if (isFirstNOP) {
+      isFirstNOP = false;
+      FirstNOPLoc = C_NOP.getLoc();
+    }
+  }
+
+  // Attach R_RISCV_ALIGN relocation type to the first NOP.
+  Fixups.push_back(MCFixup::create(
+      0, TotalBytes, MCFixupKind(RISCV::fixup_riscv_align), FirstNOPLoc));
+  ++MCNumFixups;
+
+  return MCNum;
+}
+
 void RISCVMCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
                                            SmallVectorImpl<MCFixup> &Fixups,
                                            const MCSubtargetInfo &STI) const {
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   // Get byte count of instruction.
   unsigned Size = Desc.getSize();
 
   if (MI.getOpcode() == RISCV::PseudoCALL) {
     expandFunctionCall(MI, OS, Fixups, STI);
     MCNumEmitted += 2;
     return;
+  } else if (MI.getOpcode() == RISCV::PseudoNOPs) {
+    MCNumEmitted += expandNOPs(MI, OS, Fixups, STI);
+    return;
   }
 
   switch (Size) {
   default:
     llvm_unreachable("Unhandled encodeInstruction length!");
   case 2: {
     uint16_t Bits = getBinaryCodeForInstr(MI, Fixups, STI);
     support::endian::Writer<support::little>(OS).write<uint16_t>(Bits);

lib/Target/RISCV/RISCVInstrInfo.td

 // Pessimistically assume the stack pointer will be clobbered
 let Defs = [X2], Uses = [X2] in {
 def ADJCALLSTACKDOWN : Pseudo<(outs), (ins i32imm:$amt1, i32imm:$amt2),
                               [(CallSeqStart timm:$amt1, timm:$amt2)]>;
 def ADJCALLSTACKUP   : Pseudo<(outs), (ins i32imm:$amt1, i32imm:$amt2),
                               [(CallSeqEnd timm:$amt1, timm:$amt2)]>;
 } // Defs = [X2], Uses = [X2]
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+def PseudoNOPs : Pseudo<(outs), (ins i32imm:$bytes), []>;
+
 //===----------------------------------------------------------------------===//
 // Standard extensions
 //===----------------------------------------------------------------------===//
 
 include "RISCVInstrInfoM.td"
 include "RISCVInstrInfoA.td"
 include "RISCVInstrInfoF.td"
 include "RISCVInstrInfoD.td"
 include "RISCVInstrInfoC.td"

test/MC/RISCV/align.s

+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c,+relax < %s \
+# RUN:     | llvm-objdump -d -riscv-no-aliases - \
+# RUN:     | FileCheck -check-prefix=RELAX-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c,+relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
+# RUN:     | llvm-objdump -d -riscv-no-aliases - \
+# RUN:     | FileCheck -check-prefix=INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELOC %s
+
+# We need to insert N-2 bytes NOPs for and R_RISCV_ALIGN relocation type
+# for .align N directive when linker relaxation enabled.
+# Linker could satisfy alignment by removing NOPs after linker relaxation.
+test:               
+  .p2align 2
+# RELAX-RELOC: R_RISCV_ALIGN - 0x2
+# RELAX-INST: c.nop
+  c.bnez  a0, .LBB0_2
+  mv  a0, zero
+# RELAX-RELOC: R_RISCV_ALIGN - 0x6
+# RELAX-INST: addi    zero, zero, 0
+# RELAX-INST: c.nop
+# INST: addi    zero, zero, 0
+  .p2align 3
+  add a0, a0, a1
+# RELAX-RELOC: R_RISCV_ALIGN - 0xE
+# RELAX-INST: addi    zero, zero, 0
+# RELAX-INST: addi    zero, zero, 0
+# RELAX-INST: addi    zero, zero, 0
+# RELAX-INST: c.nop
+# INST: addi    zero, zero, 0
+# INST: c.nop
+# RELOC-NOT: R_RISCV
+  .align 4
+.LBB0_2:
+  ret