RISCV: Remove shouldForceRelocation and unneeded relocations

[MaskRay]

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.

We now handle mixed relax/norelax case like
https://discourse.llvm.org/t/possible-problem-related-to-subtarget-usage/75283
and we can revert #73721

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-risc-v

Author: Fangrui Song (MaskRay)

Changes

RISCVAsmBackend::ForceRelocs is introduced to create required
relocations in mixed relax/norelax code. However, it also leads to
redundant relocations.

Consider the 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:

Follow-up to #140494

The STI->hasFeature(RISCV::FeatureRelax) condition can be replaced
with Fixup.needsRelax(), which requires updating a few tests. I want
to test this less aggressive change first.

---

Full diff: https://github.com/llvm/llvm-project/pull/140692.diff

9 Files Affected:

• (modified) llvm/include/llvm/MC/MCAssembler.h (+2)
• (modified) llvm/lib/MC/MCAssembler.cpp (+4)
• (modified) llvm/lib/MC/MCExpr.cpp (+6)
• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (-15)
• (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp (+22-6)
• (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h (+5-3)
• (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp (-7)
• (modified) llvm/test/CodeGen/RISCV/option-relax-relocation.ll (+8-2)
• (modified) llvm/test/MC/RISCV/option-relax.s (+1-9)

diff --git a/llvm/include/llvm/MC/MCAssembler.h b/llvm/include/llvm/MC/MCAssembler.h
index d463b40cbd142..becd7cf6e8d1b 100644
--- a/llvm/include/llvm/MC/MCAssembler.h
+++ b/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; }
 
diff --git a/llvm/lib/MC/MCAssembler.cpp b/llvm/lib/MC/MCAssembler.cpp
index 3a974f3591631..c2e775389735a 100644
--- a/llvm/lib/MC/MCAssembler.cpp
+++ b/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);
 
+  // Fragments sizes are final. RISC-V style linker relaxation determines
+  // whether a PC-relative fixup is truly resolved with this flag.
+  this->HasFinalLayout = true;
+
   // Evaluate and apply the fixups, generating relocation entries as necessary.
   for (MCSection &Sec : *this) {
     for (MCFragment &Frag : Sec) {
diff --git a/llvm/lib/MC/MCExpr.cpp b/llvm/lib/MC/MCExpr.cpp
index 4c159feea48f8..bfde050b5b5ed 100644
--- a/llvm/lib/MC/MCExpr.cpp
+++ b/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()) {
+        // A relaxable fragment has indeterminate size before finishLayout.
+        // Afterwards (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(
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index f64307babab07..01704e69ddd0b 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2820,21 +2820,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
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index bcab114c0ecf0..aaef249976845 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -104,9 +104,8 @@ 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.
+// If linker relaxation is enabled, emit relocation to mark the instruction as
+// shrinkable by the linker.
 bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
                                             const MCFixup &Fixup,
                                             const MCValue &Target,
@@ -124,7 +123,7 @@ bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
     break;
   }
 
-  return STI->hasFeature(RISCV::FeatureRelax) || ForceRelocs;
+  return STI->hasFeature(RISCV::FeatureRelax);
 }
 
 bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCAssembler &,
@@ -570,6 +569,18 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
   }
 }
 
+bool RISCVAsmBackend::isPCRelFixupResolved(const MCAssembler &Asm,
+                                           const MCSymbol *SymA,
+                                           const MCFragment &F) {
+  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 +624,9 @@ bool RISCVAsmBackend::evaluateTargetFixup(
   Value = Asm.getSymbolOffset(SA) + AUIPCTarget.getConstant();
   Value -= Asm.getFragmentOffset(*AUIPCDF) + AUIPCFixup->getOffset();
 
-  return AUIPCFixup->getTargetKind() == RISCV::fixup_riscv_pcrel_hi20;
+  if (AUIPCFixup->getTargetKind() != RISCV::fixup_riscv_pcrel_hi20)
+    return false;
+  return isPCRelFixupResolved(Asm, AUIPCTarget.getAddSym(), *AUIPCDF);
 }
 
 bool RISCVAsmBackend::addReloc(MCAssembler &Asm, const MCFragment &F,
@@ -659,11 +672,14 @@ bool RISCVAsmBackend::addReloc(MCAssembler &Asm, const MCFragment &F,
     return 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 (!IsResolved && Fixup.needsRelax()) {
     auto FA = MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_RELAX);
     Asm.getWriter().recordRelocation(Asm, &F, FA, MCValue::get(nullptr),
                                      FixedValueA);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
index 874cf654e7eef..e9de9ac642b11 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
+++ b/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;
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 5a81c829e3a89..c654fd2b5cbe0 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/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() {
diff --git a/llvm/test/CodeGen/RISCV/option-relax-relocation.ll b/llvm/test/CodeGen/RISCV/option-relax-relocation.ll
index 3dc5aa64bb368..e4071d16fef67 100644
--- a/llvm/test/CodeGen/RISCV/option-relax-relocation.ll
+++ b/llvm/test/CodeGen/RISCV/option-relax-relocation.ll
@@ -2,7 +2,7 @@
 ;; after linker relaxation. See https://github.com/ClangBuiltLinux/linux/issues/1965
 
 ; RUN: llc -mtriple=riscv64 -mattr=-relax -filetype=obj < %s \
-; RUN:     | llvm-objdump -d -r - | FileCheck %s
+; RUN:     | llvm-objdump -d -r - | FileCheck %s --check-prefixes=CHECK,NORELAX
 ; RUN: llc -mtriple=riscv64 -mattr=+relax -filetype=obj < %s \
 ; RUN:     | llvm-objdump -d -r - | FileCheck %s --check-prefixes=CHECK,RELAX
 
@@ -12,14 +12,20 @@
 ; CHECK-NEXT:           R_RISCV_CALL_PLT     f
 ; RELAX-NEXT:           R_RISCV_RELAX        *ABS*
 ; CHECK-NEXT:   jalr    ra
+; CHECK-NEXT:   j       {{.*}}
+; CHECK-NEXT:   j       {{.*}}
+; NORELAX-NEXT: li      a0, 0x0
+; RELAX-NEXT:           R_RISCV_JAL  .L0
 
 define dso_local noundef signext i32 @main() local_unnamed_addr #0 {
 entry:
-  callbr void asm sideeffect ".option push\0A.option norvc\0A.option norelax\0Aj $0\0A.option pop\0A", "!i"() #2
+  callbr void asm sideeffect ".option push\0A.option norvc\0A.option norelax\0Aj $0\0A.option pop\0A", "!i"()
           to label %asm.fallthrough [label %label]
 
 asm.fallthrough:                                  ; preds = %entry
   tail call void @f()
+  callbr void asm sideeffect ".option push\0A.option norvc\0A.option norelax\0Aj $0\0A.option pop\0A", "!i"()
+          to label %asm.fallthrough [label %label]
   br label %label
 
 label:                                            ; preds = %asm.fallthrough, %entry
diff --git a/llvm/test/MC/RISCV/option-relax.s b/llvm/test/MC/RISCV/option-relax.s
index 8a6a929ad0241..7b27293fe727b 100644
--- a/llvm/test/MC/RISCV/option-relax.s
+++ b/llvm/test/MC/RISCV/option-relax.s
@@ -21,15 +21,11 @@
 
 # CHECK-INST: call foo
 # CHECK-RELOC: R_RISCV_CALL_PLT foo 0x0
-# CHECK-RELOC-NOT: R_RISCV_RELAX - 0x0
+# CHECK-RELOC-NOT: R_RISCV
 call foo
 
-# CHECK-RELOC-NEXT: R_RISCV_ADD64
-# CHECK-RELOC-NEXT: R_RISCV_SUB64
 .dword .L2-.L1
-# CHECK-RELOC-NEXT: R_RISCV_JAL
 jal zero, .L1
-# CHECK-RELOC-NEXT: R_RISCV_BRANCH
 beq s1, s1, .L1
 
 .L2:
@@ -41,8 +37,6 @@ beq s1, s1, .L1
 # CHECK-RELOC-NEXT: R_RISCV_RELAX - 0x0
 call bar
 
-# CHECK-RELOC-NEXT: R_RISCV_ADD64
-# CHECK-RELOC-NEXT: R_RISCV_SUB64
 .dword .L2-.L1
 # CHECK-RELOC-NEXT: R_RISCV_JAL
 jal zero, .L1
@@ -57,8 +51,6 @@ beq s1, s1, .L1
 # CHECK-RELOC-NOT: R_RISCV_RELAX - 0x0
 call baz
 
-# CHECK-RELOC-NEXT: R_RISCV_ADD64
-# CHECK-RELOC-NEXT: R_RISCV_SUB64
 .dword .L2-.L1
 # CHECK-RELOC-NEXT: R_RISCV_JAL
 jal zero, .L1