RISCV: Remove shouldForceRelocation and unneeded relocations

Follow-up to #140494

`shouldForceRelocation` is conservative and produces redundant
relocations.

For example, RISCVAsmBackend::ForceRelocs (introduced to support mixed
relax/norelax code) leads to redundant relocations in the following
example adapted from #77436

```
.option norelax
j label
// For assembly input, RISCVAsmParser::ParseInstruction sets ForceRelocs (https://reviews.llvm.org/D46423).
// For direct object emission, RISCVELFStreamer sets ForceRelocs (#77436)
.option relax
call foo  // linker-relaxable

.option norelax
j label   // redundant relocation due to ForceRelocs
.option relax

label:
```

Root problem: The `isSymbolRefDifferenceFullyResolvedImpl` condition in
MCAssembler::evaluateFixup does not check whether two locations are
separated by a fragment whose size can be indeterminate due to linker
instruction (e.g. MCDataFragment with relaxation, or MCAlignFragment
due to indeterminate start offst).

This patch

* Updates the fragment walk code in
  `attemptToFoldSymbolOffsetDifference` to treat MCRelaxableFragment
  (for --riscv-asm-relax-branches) as fixed size after finishLayout.
* Adds a condition in `addReloc` to complement
  `isSymbolRefDifferenceFullyResolvedImpl`.
* Removes the no longer needed `shouldForceRelocation`.

This fragment walk code path handles nicely handles
mixed relax/norelax case from
https://discourse.llvm.org/t/possible-problem-related-to-subtarget-usage/75283
and allows us to remove `MCSubtargetInfo` argument (#73721) as a follow-up.

This fragment walk code should be avoided in the absence of
linker-relaxable fragments within the current section.

Adjust two bolt/test/RISCV tests (#141310)

Pull Request: #140692

Author: MaskRay

bolt/test/RISCV/reloc-label-diff.s

@@ -20,4 +20,6 @@ _test_end:
   .data
 // CHECK: Hex dump of section '.data':
 // CHECK: 0x{{.*}} 04000000
+  .reloc ., R_RISCV_ADD32, _test_end
+  .reloc ., R_RISCV_SUB32, _start
   .word _test_end - _start

bolt/test/RISCV/reorder-blocks-reverse.s

@@ -7,6 +7,7 @@
   .p2align 1
 _start:
   nop
+  .reloc ., R_RISCV_BRANCH, 1f
   beq t0, t1, 1f
   nop
   beq t0, t2, 2f

llvm/include/llvm/MC/MCAssembler.h

@@ -64,6 +64,7 @@ class MCAssembler {
   std::unique_ptr<MCObjectWriter> Writer;
 
   bool HasLayout = false;
+  bool HasFinalLayout = false;
   bool RelaxAll = false;
 
   SectionListType Sections;
@@ -197,6 +198,7 @@ class MCAssembler {
   void layout();
 
   bool hasLayout() const { return HasLayout; }
+  bool hasFinalLayout() const { return HasFinalLayout; }
   bool getRelaxAll() const { return RelaxAll; }
   void setRelaxAll(bool Value) { RelaxAll = Value; }
 

llvm/include/llvm/MC/MCFixup.h

@@ -73,9 +73,9 @@ class MCFixup {
   /// determine how the operand value should be encoded into the instruction.
   MCFixupKind Kind = FK_NONE;
 
-  /// Used by RISC-V style linker relaxation. If the fixup is unresolved,
-  /// whether a RELAX relocation should follow.
-  bool NeedsRelax = false;
+  /// Used by RISC-V style linker relaxation. Whether the fixup is
+  /// linker-relaxable.
+  bool LinkerRelaxable = false;
 
   /// Consider bit fields if we need more flags.
 
@@ -105,8 +105,8 @@ class MCFixup {
 
   const MCExpr *getValue() const { return Value; }
 
-  bool needsRelax() const { return NeedsRelax; }
-  void setNeedsRelax() { NeedsRelax = true; }
+  bool isLinkerRelaxable() const { return LinkerRelaxable; }
+  void setLinkerRelaxable() { LinkerRelaxable = true; }
 
   /// Return the generic fixup kind for a value with the given size. It
   /// is an error to pass an unsupported size.

llvm/include/llvm/MC/MCSection.h

@@ -107,6 +107,10 @@ class MCSection {
 
   bool IsVirtual : 1;
 
+  /// Whether the section contains linker-relaxable fragments. If true, the
+  /// offset between two locations may not be fully resolved.
+  bool LinkerRelaxable : 1;
+
   MCDummyFragment DummyFragment;
 
   // Mapping from subsection number to fragment list. At layout time, the
@@ -175,6 +179,9 @@ class MCSection {
   bool isRegistered() const { return IsRegistered; }
   void setIsRegistered(bool Value) { IsRegistered = Value; }
 
+  bool isLinkerRelaxable() const { return LinkerRelaxable; }
+  void setLinkerRelaxable() { LinkerRelaxable = true; }
+
   const MCDummyFragment &getDummyFragment() const { return DummyFragment; }
   MCDummyFragment &getDummyFragment() { return DummyFragment; }
 

llvm/lib/MC/MCAssembler.cpp

@@ -897,6 +897,10 @@ void MCAssembler::layout() {
   // example, to set the index fields in the symbol data).
   getWriter().executePostLayoutBinding(*this);
 
+  // Fragment sizes are finalized. For RISC-V linker relaxation, this flag
+  // helps check whether a PC-relative fixup is fully resolved.
+  this->HasFinalLayout = true;
+
   // Evaluate and apply the fixups, generating relocation entries as necessary.
   for (MCSection &Sec : *this) {
     for (MCFragment &Frag : Sec) {

llvm/lib/MC/MCELFStreamer.cpp

@@ -450,8 +450,10 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
   auto Fixups = MutableArrayRef(DF->getFixups()).slice(FixupStartIndex);
   for (auto &Fixup : Fixups) {
     Fixup.setOffset(Fixup.getOffset() + CodeOffset);
-    if (Fixup.needsRelax())
+    if (Fixup.isLinkerRelaxable()) {
       DF->setLinkerRelaxable();
+      getCurrentSectionOnly()->setLinkerRelaxable();
+    }
   }
 
   DF->setHasInstructions(STI);

llvm/lib/MC/MCExpr.cpp

@@ -390,6 +390,12 @@ static void attemptToFoldSymbolOffsetDifference(const MCAssembler *Asm,
       unsigned Count;
       if (DF) {
         Displacement += DF->getContents().size();
+      } else if (auto *RF = dyn_cast<MCRelaxableFragment>(FI);
+                 RF && Asm->hasFinalLayout()) {
+        // Before finishLayout, a relaxable fragment's size is indeterminate.
+        // After layout, during relocation generation, it can be treated as a
+        // data fragment.
+        Displacement += RF->getContents().size();
       } else if (auto *AF = dyn_cast<MCAlignFragment>(FI);
                  AF && Layout && AF->hasEmitNops() &&
                  !Asm->getBackend().shouldInsertExtraNopBytesForCodeAlign(

llvm/lib/MC/MCSection.cpp

@@ -23,7 +23,7 @@ MCSection::MCSection(SectionVariant V, StringRef Name, bool IsText,
                      bool IsVirtual, MCSymbol *Begin)
     : Begin(Begin), BundleGroupBeforeFirstInst(false), HasInstructions(false),
       HasLayout(false), IsRegistered(false), IsText(IsText),
-      IsVirtual(IsVirtual), Name(Name), Variant(V) {
+      IsVirtual(IsVirtual), LinkerRelaxable(false), Name(Name), Variant(V) {
   DummyFragment.setParent(this);
   // The initial subsection number is 0. Create a fragment list.
   CurFragList = &Subsections.emplace_back(0u, FragList{}).second;

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp

@@ -504,7 +504,7 @@ bool LoongArchAsmBackend::addReloc(MCAssembler &Asm, const MCFragment &F,
   IsResolved = Fallback();
   // If linker relaxation is enabled and supported by the current relocation,
   // append a RELAX relocation.
-  if (Fixup.needsRelax()) {
+  if (Fixup.isLinkerRelaxable()) {
     auto FA = MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_LARCH_RELAX);
     Asm.getWriter().recordRelocation(Asm, &F, FA, MCValue::get(nullptr),
                                      FixedValueA);

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp

@@ -203,7 +203,7 @@ LoongArchMCCodeEmitter::getExprOpValue(const MCInst &MI, const MCOperand &MO,
   // a bit so that if fixup is unresolved, a R_LARCH_RELAX relocation will be
   // appended.
   if (EnableRelax && RelaxCandidate)
-    Fixups.back().setNeedsRelax();
+    Fixups.back().setLinkerRelaxable();
 
   return 0;
 }
@@ -254,7 +254,7 @@ void LoongArchMCCodeEmitter::expandAddTPRel(const MCInst &MI,
   Fixups.push_back(
       MCFixup::create(0, Expr, ELF::R_LARCH_TLS_LE_ADD_R, MI.getLoc()));
   if (STI.hasFeature(LoongArch::FeatureRelax))
-    Fixups.back().setNeedsRelax();
+    Fixups.back().setLinkerRelaxable();
 
   // Emit a normal ADD instruction with the given operands.
   unsigned ADD = MI.getOpcode() == LoongArch::PseudoAddTPRel_D

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

@@ -2885,21 +2885,6 @@ bool RISCVAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) {
 bool RISCVAsmParser::parseInstruction(ParseInstructionInfo &Info,
                                       StringRef Name, SMLoc NameLoc,
                                       OperandVector &Operands) {
-  // Ensure that if the instruction occurs when relaxation is enabled,
-  // relocations are forced for the file. Ideally this would be done when there
-  // is enough information to reliably determine if the instruction itself may
-  // cause relaxations. Unfortunately instruction processing stage occurs in the
-  // same pass as relocation emission, so it's too late to set a 'sticky bit'
-  // for the entire file.
-  if (getSTI().hasFeature(RISCV::FeatureRelax)) {
-    auto *Assembler = getTargetStreamer().getStreamer().getAssemblerPtr();
-    if (Assembler != nullptr) {
-      RISCVAsmBackend &MAB =
-          static_cast<RISCVAsmBackend &>(Assembler->getBackend());
-      MAB.setForceRelocs();
-    }
-  }
-
   // Apply mnemonic aliases because the destination mnemonic may have require
   // custom operand parsing. The generic tblgen'erated code does this later, at
   // the start of MatchInstructionImpl(), but that's too late for custom

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

@@ -14,6 +14,7 @@
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCExpr.h"
+#include "llvm/MC/MCFragment.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCValue.h"
@@ -104,29 +105,6 @@ MCFixupKindInfo RISCVAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
   return Infos[Kind - FirstTargetFixupKind];
 }
 
-// If linker relaxation is enabled, or the relax option had previously been
-// enabled, always emit relocations even if the fixup can be resolved. This is
-// necessary for correctness as offsets may change during relaxation.
-bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
-                                            const MCFixup &Fixup,
-                                            const MCValue &Target,
-                                            const MCSubtargetInfo *STI) {
-  switch (Fixup.getTargetKind()) {
-  default:
-    break;
-  case FK_Data_1:
-  case FK_Data_2:
-  case FK_Data_4:
-  case FK_Data_8:
-  case FK_Data_leb128:
-    if (Target.isAbsolute())
-      return false;
-    break;
-  }
-
-  return STI->hasFeature(RISCV::FeatureRelax) || ForceRelocs;
-}
-
 bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCAssembler &,
                                                    const MCFixup &Fixup,
                                                    const MCValue &,
@@ -570,6 +548,27 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
   }
 }
 
+bool RISCVAsmBackend::isPCRelFixupResolved(const MCAssembler &Asm,
+                                           const MCSymbol *SymA,
+                                           const MCFragment &F) {
+  // If the section does not contain linker-relaxable instructions, PC-relative
+  // fixups can be resolved.
+  if (!F.getParent()->isLinkerRelaxable())
+    return true;
+
+  // Otherwise, check if the offset between the symbol and fragment is fully
+  // resolved, unaffected by linker-relaxable fragments (e.g. instructions or
+  // offset-affected MCAlignFragment). Complements the generic
+  // isSymbolRefDifferenceFullyResolvedImpl.
+  if (!PCRelTemp)
+    PCRelTemp = Asm.getContext().createTempSymbol();
+  PCRelTemp->setFragment(const_cast<MCFragment *>(&F));
+  MCValue Res;
+  MCExpr::evaluateSymbolicAdd(&Asm, false, MCValue::get(SymA),
+                              MCValue::get(nullptr, PCRelTemp), Res);
+  return !Res.getSubSym();
+}
+
 bool RISCVAsmBackend::evaluateTargetFixup(
     const MCAssembler &Asm, const MCFixup &Fixup, const MCFragment *DF,
     const MCValue &Target, const MCSubtargetInfo *STI, uint64_t &Value) {
@@ -613,7 +612,8 @@ bool RISCVAsmBackend::evaluateTargetFixup(
   Value = Asm.getSymbolOffset(SA) + AUIPCTarget.getConstant();
   Value -= Asm.getFragmentOffset(*AUIPCDF) + AUIPCFixup->getOffset();
 
-  return AUIPCFixup->getTargetKind() == RISCV::fixup_riscv_pcrel_hi20;
+  return AUIPCFixup->getTargetKind() == RISCV::fixup_riscv_pcrel_hi20 &&
+         isPCRelFixupResolved(Asm, AUIPCTarget.getAddSym(), *AUIPCDF);
 }
 
 bool RISCVAsmBackend::addReloc(MCAssembler &Asm, const MCFragment &F,
@@ -659,11 +659,17 @@ bool RISCVAsmBackend::addReloc(MCAssembler &Asm, const MCFragment &F,
     return false;
   }
 
+  // If linker relaxation is enabled and supported by the current relocation,
+  // generate a relocation and then append a RELAX.
+  if (Fixup.isLinkerRelaxable())
+    IsResolved = false;
+  if (IsResolved &&
+      (getFixupKindInfo(Fixup.getKind()).Flags & MCFixupKindInfo::FKF_IsPCRel))
+    IsResolved = isPCRelFixupResolved(Asm, Target.getAddSym(), F);
   IsResolved = MCAsmBackend::addReloc(Asm, F, Fixup, Target, FixedValue,
                                       IsResolved, STI);
-  // If linker relaxation is enabled and supported by the current relocation,
-  // append a RELAX relocation.
-  if (Fixup.needsRelax()) {
+
+  if (Fixup.isLinkerRelaxable()) {
     auto FA = MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_RELAX);
     Asm.getWriter().recordRelocation(Asm, &F, FA, MCValue::get(nullptr),
                                      FixedValueA);

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

@@ -25,16 +25,18 @@ class RISCVAsmBackend : public MCAsmBackend {
   const MCSubtargetInfo &STI;
   uint8_t OSABI;
   bool Is64Bit;
-  bool ForceRelocs = false;
   const MCTargetOptions &TargetOptions;
+  // Temporary symbol used to check whether a PC-relative fixup is resolved.
+  MCSymbol *PCRelTemp = nullptr;
+
+  bool isPCRelFixupResolved(const MCAssembler &Asm, const MCSymbol *SymA,
+                            const MCFragment &F);
 
 public:
   RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit,
                   const MCTargetOptions &Options);
   ~RISCVAsmBackend() override = default;
 
-  void setForceRelocs() { ForceRelocs = true; }
-
   // Return Size with extra Nop Bytes for alignment directive in code section.
   bool shouldInsertExtraNopBytesForCodeAlign(const MCAlignFragment &AF,
                                              unsigned &Size) override;
@@ -60,10 +62,6 @@ class RISCVAsmBackend : public MCAsmBackend {
   std::unique_ptr<MCObjectTargetWriter>
   createObjectTargetWriter() const override;
 
-  bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
-                             const MCValue &Target,
-                             const MCSubtargetInfo *STI) override;
-
   bool fixupNeedsRelaxationAdvanced(const MCAssembler &,
                                     const MCFixup &, const MCValue &, uint64_t,
                                     bool) const override;

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

@@ -34,13 +34,6 @@ RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
   setTargetABI(RISCVABI::computeTargetABI(STI.getTargetTriple(), Features,
                                           MAB.getTargetOptions().getABIName()));
   setFlagsFromFeatures(STI);
-  // `j label` in `.option norelax; j label; .option relax; ...; label:` needs a
-  // relocation to ensure the jump target is correct after linking. This is due
-  // to a limitation that shouldForceRelocation has to make the decision upfront
-  // without knowing a possibly future .option relax. When RISCVAsmParser is used,
-  // its ParseInstruction may call setForceRelocs as well.
-  if (STI.hasFeature(RISCV::FeatureRelax))
-    static_cast<RISCVAsmBackend &>(MAB).setForceRelocs();
 }
 
 RISCVELFStreamer &RISCVTargetELFStreamer::getStreamer() {

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

@@ -212,7 +212,7 @@ void RISCVMCCodeEmitter::expandAddTPRel(const MCInst &MI,
   Fixups.push_back(
       MCFixup::create(0, Expr, ELF::R_RISCV_TPREL_ADD, MI.getLoc()));
   if (STI.hasFeature(RISCV::FeatureRelax))
-    Fixups.back().setNeedsRelax();
+    Fixups.back().setLinkerRelaxable();
 
   // Emit a normal ADD instruction with the given operands.
   MCInst TmpInst = MCInstBuilder(RISCV::ADD)
@@ -654,7 +654,7 @@ uint64_t RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
   // a bit so that if fixup is unresolved, a R_RISCV_RELAX relocation will be
   // appended.
   if (EnableRelax && RelaxCandidate)
-    Fixups.back().setNeedsRelax();
+    Fixups.back().setLinkerRelaxable();
   ++MCNumFixups;
 
   return 0;

llvm/test/CodeGen/RISCV/option-relax-relocation.ll

@@ -13,7 +13,6 @@
 ; RELAX-NEXT:           R_RISCV_RELAX        *ABS*
 ; CHECK-NEXT:   jalr    ra
 ; CHECK-NEXT:   j       {{.*}}
-; RELAX-NEXT:           R_RISCV_JAL  .LBB0_{{.*}}
 ; CHECK-NEXT:   j       {{.*}}
 ; RELAX-NEXT:           R_RISCV_JAL  .L0
 ; NORELAX-NEXT: li      a0, 0x0

llvm/test/MC/RISCV/fixups-binary-expression.s

@@ -21,15 +21,14 @@ beq a0, a1, .LBB1+32
 
 c.j     .+32
 # CHECK-INSTR: c.j   0x30
-# CHECK-RELOC-NEXT:  R_RISCV_RVC_JUMP
 
 c.j     .LBB2+4
 # CHECK-INSTR: c.j   0x22
 # CHECK-RELOC-NEXT:      R_RISCV_RVC_JUMP
 
 c.beqz a0, .-2
 # CHECK-INSTR: c.beqz a0, 0x12
-# CHECK-RELOC-NEXT:  R_RISCV_RVC_BRANCH
 
 call relax
+# CHECK-RELOC-NEXT:  R_RISCV_CALL_PLT
 .LBB2:

llvm/test/MC/RISCV/linker-relaxation.s

@@ -58,14 +58,17 @@ bar:
 
 beq s1, s1, bar
 # NORELAX-RELOC-NOT: R_RISCV_BRANCH
-# RELAX-RELOC: R_RISCV_BRANCH bar 0x0
 
 call bar
 # NORELAX-RELOC-NOT: R_RISCV_CALL
 # NORELAX-RELOC-NOT: R_RISCV_RELAX
-# RELAX-RELOC: R_RISCV_CALL_PLT bar 0x0
+# RELAX-RELOC-NEXT: R_RISCV_CALL_PLT bar 0x0
 # RELAX-RELOC: R_RISCV_RELAX - 0x0
 
+beq s1, s1, bar
+# NORELAX-RELOC-NOT: R_RISCV_BRANCH
+# RELAX-RELOC-NEXT: R_RISCV_BRANCH bar 0x0
+
 lui t1, %hi(bar)
 # NORELAX-RELOC: R_RISCV_HI20 bar 0x0
 # NORELAX-RELOC-NOT: R_RISCV_RELAX