RISCV: adjust handling of relocation emission for RISCV

[compnerd]

Summary

This re-architects the RISCV relocation handling to bring the
implementation closer in line with the implementation in binutils. We
would previously aggressively resolve the relocation. With this
restructuring, we always will emit a paired relocation for any symbolic
difference of the type of S±T[±C] where S and T are labels and C is a
constant.

GAS has a special target hook controlled by RELOC_EXPANSION_POSSIBLE
which indicates that a fixup may be expanded into multiple relocations.
This is used by the RISCV backend to always emit a paired relocation -
either ADD[WIDTH] + SUB[WIDTH] for text relocations or SET[WIDTH] +
SUB[WIDTH] for a debug info relocation. Irrespective of whether linker
relaxation support is enabled, symbolic difference is always emitted as
a paired relocation.

This change also sinks the target specific behaviour down into the
target specific area rather than exposing it to the shared relocation
handling. In the process, we also sink the "special" handling for debug
information down into the RISCV target. Although this improves the path
for the other targets, this is not necessarily entirely ideal either.
The changes in the debug info emission could be done through another
type of hook as this functionality would be required by any other target
which wishes to do linker relaxation. However, as there are no other
targets in LLVM which currently do this, this is a reasonable thing to
do until such time as the code needs to be shared.

Improve the handling of the relocation (and add a reduced test case from
the Linux kernel) to ensure that we handle complex expressions for
symbolic difference. This ensures that we correct relocate symbols with
the adddends normalized and associated with the addition portion of the
paired relocation.

This change also addresses some review comments from Alex Bradbury about
the relocations meant for use in the DWARF CFA being named incorrectly
(using ADD6 instead of SET6) in the original change which introduced the
relocation type.

This resolves the issues with the symbolic difference emission
sufficiently to enable building the Linux kernel with clang+IAS+lld
(without linker relaxation).

Resolves PR50153, PR50156!
Fixes: ClangBuiltLinux/linux#1023, ClangBuiltLinux/linux#1143

[MaskRay]

Thanks for working on this. requiresDiffExpressionRelocations FK_Data_Add_1 canFold ... are all RISC-V specific layering violation concepts which add significant complexity to the MC component.
It is good to see to see them simplified and moved to the proper place (e.g. lib/Target/RISCV/MCTargetDesc as your diff shows).

[jrtc27]

Could you please re-upload the diff with full context?

[jrtc27]

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

396

Please don't "fix" unrelated whitespace

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

56

Ditto

156–161

I'm confused why this doesn't overwrite RI again with an R_RISCV_SUB8?

160

This approach is also going to give quadratic complexity. Is there a reason we can't emplace_back? If so, can a stable sort post-processing step not fix that more efficiently (asymptotically)?

163

Please no comma operators outside of for

[jrtc27]

llvm/lib/MC/ELFObjectWriter.cpp

1522–1526

Why is this not a hook?

[jrtc27]

This undoes an ancient optimization in LLVM to support RISC-V's desire to over-emit relocations for link time relocation relaxation.

Do you mean that we currently emit more relocations than we need to, or is this snark directed towards RISC-V's linker relaxations? If the latter, please remove such comments from your patch description.

[compnerd]

llvm/lib/MC/ELFObjectWriter.cpp

1522–1526

This might make sense to be a hook, however, Im not sure if we need a new hook, I think that there are hooks that give us the necessary information, just need to wire them up.

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

396

Sure, from a final patch perhaps, but when working on something its very helpful.

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

156–161

Because there are two relocations which need to be emitted. The problem here is that we need a second relocation on a different symbol. The ideal thing would be that we generate two fixups in the first place.

160

Yeah, I suppose we can do an emplace_back and sort at the end.

[jrtc27]

In D103539#2794189, @jrtc27 wrote:

Could you please re-upload the diff with full context?

Still applies.

In D103539#2794400, @jrtc27 wrote:

This undoes an ancient optimization in LLVM to support RISC-V's desire to over-emit relocations for link time relocation relaxation.

Do you mean that we currently emit more relocations than we need to, or is this snark directed towards RISC-V's linker relaxations? If the latter, please remove such comments from your patch description.

Still applies.

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

396

Only locally. For reviewing it's a nuisance, so please remove them from the review.

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

155

Use std::pair or a local struct type

156–161

I know what the code is trying to do, what I'm failing to grasp is why it actually works, it looks buggy to me, as in that it doesn't do what I think it does and what you say it's meant to, so I must be missing something given the tests still pass (I assume they do given you haven't touched any?).

163

Still applies below.

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

61

Why has this suddenly appeared in version 2?

[compnerd]

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

396

This isn't meant for reviewing yet :) - its marked as a WIP.

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

61

This change isn't really ready for review yet. I wanted to share it with @MaskRay so I put it here as it is where it is going to end up. I'm still working through the changes, so expect a lot of major changes. Thats also why there is no one tagged on the diff.

[compnerd]

Closer to passing tests (9 more failures to understand). It matches gas' behaviour better, is more uniformly behaved.

[jrtc27]

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

19

There's no longer a RUN line for this... also this test makes no sense in the first place, it's trying to use 64-bit relocations on RV32. That should just be an error.

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

12 ↗

(On Diff #349921)

Again, no longer used by a RUN line. And this looks like a bug, we're passing --mc-relax-all to the assembler, so this branch should have been relaxed in the branch relaxation sense (which is not the same as the linker relaxation sense), no? GNU as doesn't have --mc-relax-all, comparing against it is meaningless.

[compnerd]

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

19

I didn't even notice that the relocation types are just flat out wrong. That seems like a separate issue.

I wanted to keep the now dead check to do a last round of verification of the changes, at which point it should be removed.

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

12 ↗

(On Diff #349921)

Similar to above, I wanted to keep it until I do the final sweep through the change.

Agreed, that matching the behaviour with GAS for this does not make sense. I will have to adjust the implementation to accommodate this once I figure out the remaining cases.

It does raise the question of does it make sense for us to have a -mrelax and a -mc-relax-all? Perhaps we should just treat -mrelax as -mc-relax-all. Is there a heavy penalty for this? Yes, linking may take a little bit more time and memory and the object file size will be slightly larger, but there shouldn't be any runtime cost. The question boils down to, is the slight link time savings worth the two separate paths.

[simoncook]

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

19

The relocations are 64-bit because of the size of .dword, I don't think this is an error?

[jrtc27]

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

19

The relocations match the directive, but the directive itself makes no sense on RV32. Can you do .8byte foo on i386?...

19

And if you did want to support it, I'd argue it should just be a zero-padded 32-bit relocation (i.e. offset by 4 bytes on big-endian).

[compnerd]

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

19

Well, I don't think that we should change that behaviour in this change. Furthermore, it will require working with GCC and binutils as it currently _does_ generate a pair of R_RISCV_ADD64 and R_RISCV_SUB64 on RV32.

[compnerd]

7 failures to look into; need to still replace the use of RelaxAll which would also improve the test pass rate.

[compnerd]

Down to 6 failures to explore.

Various clean ups to the change. This almost is inline with what GAS does to handle the symbolic differences. This is missing a specific tweak to the condition where the difference is constrained immediately instead of being relaxed (that is, we are over-relaxing currently).

[compnerd]

3 failures left now. KI:

• 6B relocations are not implemented
• debug_line references aren't correctly emitted

One thing to note is that this seems to improve the generation of the eh_frame, which is important for enabling relaxation without negatively impacting unwinding.

[compnerd]

2 remaining failures KI:

• debug_line encoding is not relaxable

[nickdesaulniers]

With Diff 351900 of this patch applied, a RISCV build of the mainline Linux kernel with LLVM=1 LLVM_IAS=1 builds and boots. This patch resolves https://github.com/ClangBuiltLinux/linux/issues/1023 for me.

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

18

Unused?

[compnerd]

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

18

Ugh, I missed this :-(; Yes, it is now unused, at one point I was using it for std::pair. Thanks for pointing that out.

[nickdesaulniers]

With Diff 352163 of this patch applied, I can build+boot a mainline riscv Linux kernel.
https://github.com/ClangBuiltLinux/linux/issues/1023 is resolved, and further now so is https://github.com/ClangBuiltLinux/linux/issues/1143. This is gets us back to being able to build entirely with LLVM (clang, clang's integrated assembler, LLD). That's huge! Thanks @compnerd !

I also built the kernel with debug info; llvm-dwarfdump didn't have any complaints about the input.

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

195

does this need to be static?

208

Should this call getSymB() rather than getSymA()?

230–233

Should this be:

MCStreamer::emitValueImpl(Value, Size, Loc);
if (!useLinkerRelaxation(getContext(), Value, A, B))
  return;

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

105–120

Some of the comments have an extra R in the relocation type identifier.

llvm/test/MC/RISCV/hilo-constaddr-expr.s

4 ↗

(On Diff #352163)

is it ok to drop the llvm-objdump -d test?

[jrtc27]

llvm/test/MC/RISCV/hilo-constaddr-expr.s

4 ↗

(On Diff #352163)

It was there to ensure that the assembled output was correct, but if it no longer assembles then you can't exactly test it.

[MaskRay]

llvm/test/CodeGen/RISCV/fixups-diff.ll

27 ↗

(On Diff #352163)

Add -NEXT:

Otherwise CHECK: } cannot ensure there is no additional ADD/SUB.

33 ↗

(On Diff #352163)

ditto

llvm/test/MC/RISCV/hilo-constaddr-expr.s

12 ↗

(On Diff #352163)

nit: [[@ is deprecated. use :[[#@LINE+1]]:[[#]]: error: ...

llvm/test/MC/RISCV/reloc-addend.s

10 ↗

(On Diff #352163)

-NEXT

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

1 ↗

(On Diff #352163)

riscv64-unknown-none-elf can be simplified as riscv64

30 ↗

(On Diff #352163)

To ensure no additional add/sub, add -NEXT and check } wherever appropriate

[MaskRay]

llvm/test/MC/RISCV/fixups-expr.s

2

Since you are switching the check-prefix styles...

The most common form is --check-prefix=

llvm/test/MC/RISCV/hilo-constaddr-expr.s

12 ↗

(On Diff #352163)

I think it is useful to track the column information ([[#]] -> an integer). This ensures that the reported diagnostic makes sense.

[compnerd]

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

195

No, it doesn't need to be static. It is more at a conceptual level not something tied into the state of the streamer - it simply looks at an expression and tells you if you need a paired relocation for handling this. BTW, I welcome bike-shedding on this function's name - I dislike the current name of this function.

208

Move along, nothing to see here. That is rather embarrassing. Thank you for flagging this.

230–233

No, they are going down different functions - MCELFStreamer vs MCStreamer.

[nickdesaulniers]

LGTM; big thanks for sorting all of this out!

[MaskRay]

llvm/lib/MC/MCAssembler.cpp

1086

1111

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

236

The old comment is unintentionally dropped: // According to DWARF spec., the DW_LNS_fixed_advance_pc opcode takes a ..

s/50000/60000/ to restore the previous threshold.

277

blank line seems redundant

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

98

s/WasRelaxed/Relaxed/ to be consistent with the impl

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

84

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

"Don’t duplicate function or class name at the beginning of the comment. "

Just ignore some other comments which violate this rule.

[compnerd]

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

98

Changed the implementation instead.

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

84

*shrug* sure, though I think that consistency is better than following the rules in this case (and is in the developer guidelines for LLVM IIRC). However, I don't feel strongly enough to bother arguing ... so changed. Also the comment leader doesn't seem like doxygen will heed it.

[jrtc27]

_sigh_ forgot to submit these review comments

llvm/test/CodeGen/RISCV/fixups-diff.ll

25–27 ↗

(On Diff #352264)

Please line things up, and ideally also indent within { (matching the relative indentation of the output) to make it clearer

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

1–4

No need for a custom prefix

15

-NEXT?

[compnerd]

I'll address the clean ups in a follow up.

[compnerd]

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

1–4

Excellent point!

15

Good idea.

[compnerd]

e70d4994ea9eb44ba5a86c5073ac83cf500aabdd should have the suggestions incorporated.

llvm/include/llvm/MC/MCAsmBackend.h

                           uint64_t Value, bool IsResolved,
                           const MCSubtargetInfo *STI) const = 0;
 
-  /// Check whether the given target requires emitting differences of two
-  /// symbols as a set of relocations.
-  virtual bool requiresDiffExpressionRelocations() const { return false; }
-
   /// @}
 
   /// \name Target Relaxation Interfaces

llvm/include/llvm/MC/MCAssembler.h

   /// By default it's 0, which means bundling is disabled.
   unsigned BundleAlignSize;
 
+  bool Relax : 1;
   bool RelaxAll : 1;
   bool SubsectionsViaSymbols : 1;
   bool IncrementalLinkerCompatible : 1;

     IncrementalLinkerCompatible = Value;
   }
 
+  bool getRelax() const { return Relax; }
+  void setRelax(bool Value) { Relax = Value; }
+
   bool getRelaxAll() const { return RelaxAll; }
   void setRelaxAll(bool Value) { RelaxAll = Value; }
 

llvm/include/llvm/MC/MCFixup.h

   FK_SecRel_2,    ///< A two-byte section relative fixup.
   FK_SecRel_4,    ///< A four-byte section relative fixup.
   FK_SecRel_8,    ///< A eight-byte section relative fixup.
-  FK_Data_Add_1,  ///< A one-byte add fixup.
-  FK_Data_Add_2,  ///< A two-byte add fixup.
-  FK_Data_Add_4,  ///< A four-byte add fixup.
-  FK_Data_Add_8,  ///< A eight-byte add fixup.
-  FK_Data_Add_6b, ///< A six-bits add fixup.
-  FK_Data_Sub_1,  ///< A one-byte sub fixup.
-  FK_Data_Sub_2,  ///< A two-byte sub fixup.
-  FK_Data_Sub_4,  ///< A four-byte sub fixup.
-  FK_Data_Sub_8,  ///< A eight-byte sub fixup.
-  FK_Data_Sub_6b, ///< A six-bits sub fixup.
 
   FirstTargetFixupKind = 128,
 

     return FI;
   }
 
-  /// Return a fixup corresponding to the add half of a add/sub fixup pair for
-  /// the given Fixup.
-  static MCFixup createAddFor(const MCFixup &Fixup) {
-    MCFixup FI;
-    FI.Value = Fixup.getValue();
-    FI.Offset = Fixup.getOffset();
-    FI.Kind = getAddKindForKind(Fixup.getKind());
-    FI.Loc = Fixup.getLoc();
-    return FI;
-  }
-
-  /// Return a fixup corresponding to the sub half of a add/sub fixup pair for
-  /// the given Fixup.
-  static MCFixup createSubFor(const MCFixup &Fixup) {
-    MCFixup FI;
-    FI.Value = Fixup.getValue();
-    FI.Offset = Fixup.getOffset();
-    FI.Kind = getSubKindForKind(Fixup.getKind());
-    FI.Loc = Fixup.getLoc();
-    return FI;
-  }
-
   MCFixupKind getKind() const { return Kind; }
 
   unsigned getTargetKind() const { return Kind; }

     }
   }
 
-  /// Return the generic fixup kind for an addition with a given size. It
-  /// is an error to pass an unsupported size.
-  static MCFixupKind getAddKindForKind(MCFixupKind Kind) {
-    switch (Kind) {
-    default: llvm_unreachable("Unknown type to convert!");
-    case FK_Data_1: return FK_Data_Add_1;
-    case FK_Data_2: return FK_Data_Add_2;
-    case FK_Data_4: return FK_Data_Add_4;
-    case FK_Data_8: return FK_Data_Add_8;
-    case FK_Data_6b: return FK_Data_Add_6b;
-    }
-  }
-
-  /// Return the generic fixup kind for an subtraction with a given size. It
-  /// is an error to pass an unsupported size.
-  static MCFixupKind getSubKindForKind(MCFixupKind Kind) {
-    switch (Kind) {
-    default: llvm_unreachable("Unknown type to convert!");
-    case FK_Data_1: return FK_Data_Sub_1;
-    case FK_Data_2: return FK_Data_Sub_2;
-    case FK_Data_4: return FK_Data_Sub_4;
-    case FK_Data_8: return FK_Data_Sub_8;
-    case FK_Data_6b: return FK_Data_Sub_6b;
-    }
-  }
-
   SMLoc getLoc() const { return Loc; }
 };
 

llvm/lib/MC/ELFObjectWriter.cpp

         SymA.getType() == ELF::STT_GNU_IFUNC)
       return false;
   }
+  // RISC-V requires the relocation to be split up to handle the ADD_SUB split
+  // relocation.  These are required to ensure that symbolic differences are
+  // emitted as paired ADD/SUB relocations.
+  if (!InSet && TargetObjectWriter->getEMachine() == ELF::EM_RISCV)
+    return !Asm.getRelax();
   return MCObjectWriter::isSymbolRefDifferenceFullyResolvedImpl(Asm, SymA, FB,
                                                                 InSet, IsPCRel);
 }

llvm/lib/MC/MCAsmBackend.cpp

       {"FK_SecRel_2", 0, 16, 0},
       {"FK_SecRel_4", 0, 32, 0},
       {"FK_SecRel_8", 0, 64, 0},
-      {"FK_Data_Add_1", 0, 8, 0},
-      {"FK_Data_Add_2", 0, 16, 0},
-      {"FK_Data_Add_4", 0, 32, 0},
-      {"FK_Data_Add_8", 0, 64, 0},
-      {"FK_Data_Add_6b", 0, 6, 0},
-      {"FK_Data_Sub_1", 0, 8, 0},
-      {"FK_Data_Sub_2", 0, 16, 0},
-      {"FK_Data_Sub_4", 0, 32, 0},
-      {"FK_Data_Sub_8", 0, 64, 0},
-      {"FK_Data_Sub_6b", 0, 6, 0}};
+  };
 
   assert((size_t)Kind <= array_lengthof(Builtins) && "Unknown fixup kind");
   return Builtins[Kind];

llvm/lib/MC/MCAssembler.cpp

                          std::unique_ptr<MCObjectWriter> Writer)
     : Context(Context), Backend(std::move(Backend)),
       Emitter(std::move(Emitter)), Writer(std::move(Writer)),
-      BundleAlignSize(0), RelaxAll(false), SubsectionsViaSymbols(false),
-      IncrementalLinkerCompatible(false), ELFHeaderEFlags(0) {
+      BundleAlignSize(0), Relax(false), RelaxAll(false),
+      SubsectionsViaSymbols(false), IncrementalLinkerCompatible(false),
+      ELFHeaderEFlags(0) {
   VersionInfo.Major = 0; // Major version == 0 for "none specified"
 }
 

     // The fixup was unresolved, we need a relocation. Inform the object
     // writer of the relocation, and give it an opportunity to adjust the
     // fixup value if need be.
-    if (Target.getSymA() && Target.getSymB() &&
-        getBackend().requiresDiffExpressionRelocations()) {
-      // The fixup represents the difference between two symbols, which the
-      // backend has indicated must be resolved at link time. Split up the fixup
-      // into two relocations, one for the add, and one for the sub, and emit
-      // both of these. The constant will be associated with the add half of the
-      // expression.
-      MCFixup FixupAdd = MCFixup::createAddFor(Fixup);
-      MCValue TargetAdd =
-          MCValue::get(Target.getSymA(), nullptr, Target.getConstant());
-      getWriter().recordRelocation(*this, Layout, &F, FixupAdd, TargetAdd,
-                                   FixedValue);
-      MCFixup FixupSub = MCFixup::createSubFor(Fixup);
-      MCValue TargetSub = MCValue::get(Target.getSymB());
-      getWriter().recordRelocation(*this, Layout, &F, FixupSub, TargetSub,
-                                   FixedValue);
-    } else {
-      getWriter().recordRelocation(*this, Layout, &F, Fixup, Target,
-                                   FixedValue);
-    }
+    getWriter().recordRelocation(*this, Layout, &F, Fixup, Target, FixedValue);
   }
   return std::make_tuple(Target, FixedValue, IsResolved);
 }

   raw_svector_ostream OSE(Data);
   DF.getFixups().clear();
 
-  if (!getBackend().requiresDiffExpressionRelocations()) {
-    MCDwarfLineAddr::Encode(Context, getDWARFLinetableParams(), LineDelta,
-                            AddrDelta, OSE);
-  } else {
-    uint32_t Offset;
-    uint32_t Size;
-    bool SetDelta;
-    std::tie(Offset, Size, SetDelta) =
-        MCDwarfLineAddr::fixedEncode(Context, LineDelta, AddrDelta, OSE);
-    // Add Fixups for address delta or new address.
-    const MCExpr *FixupExpr;
-    if (SetDelta) {
-      FixupExpr = &DF.getAddrDelta();
-    } else {
-      const MCBinaryExpr *ABE = cast<MCBinaryExpr>(&DF.getAddrDelta());
-      FixupExpr = ABE->getLHS();
-    }
-    DF.getFixups().push_back(
-        MCFixup::create(Offset, FixupExpr,
-                        MCFixup::getKindForSize(Size, false /*isPCRel*/)));
-  }
-
+  MCDwarfLineAddr::Encode(Context, getDWARFLinetableParams(), LineDelta,
+                          AddrDelta, OSE);
   return OldSize != Data.size();
 }
 

   raw_svector_ostream OSE(Data);
   DF.getFixups().clear();
 
-  if (getBackend().requiresDiffExpressionRelocations()) {
-    uint32_t Offset;
-    uint32_t Size;
-    MCDwarfFrameEmitter::EncodeAdvanceLoc(Context, AddrDelta, OSE, &Offset,
-                                          &Size);
-    if (Size) {
-      DF.getFixups().push_back(MCFixup::create(
-          Offset, &DF.getAddrDelta(),
-          MCFixup::getKindForSizeInBits(Size /*In bits.*/, false /*isPCRel*/)));
-    }
-  } else {
-    MCDwarfFrameEmitter::EncodeAdvanceLoc(Context, AddrDelta, OSE);
-  }
-
+  MCDwarfFrameEmitter::EncodeAdvanceLoc(Context, AddrDelta, OSE);
   return OldSize != Data.size();
 }
 

   raw_svector_ostream OSE(Data);
   PF.getFixups().clear();
 
-  // Relocations should not be needed in general except on RISC-V which we are
-  // not targeted for now.
-  assert(!getBackend().requiresDiffExpressionRelocations() &&
-         "cannot relax relocations");
   // AddrDelta is a signed integer
   encodeSLEB128(AddrDelta, OSE, OldSize);
   return OldSize != Data.size();

llvm/lib/MC/MCExpr.cpp

   }
 }
 
-static bool canFold(const MCAssembler *Asm, const MCSymbolRefExpr *A,
-                    const MCSymbolRefExpr *B, bool InSet) {
-  if (InSet)
-    return true;
-
-  if (!Asm->getBackend().requiresDiffExpressionRelocations())
-    return true;
-
-  const MCSymbol &CheckSym = A ? A->getSymbol() : B->getSymbol();
-  if (!CheckSym.isInSection())
-    return true;
-
-  if (!CheckSym.getSection().hasInstructions())
-    return true;
-
-  return false;
-}
-
 /// Evaluate the result of an add between (conceptually) two MCValues.
 ///
 /// This routine conceptually attempts to construct an MCValue:

   assert((!Layout || Asm) &&
          "Must have an assembler object if layout is given!");
 
-  // If we have a layout, we can fold resolved differences. Do not do this if
-  // the backend requires this to be emitted as individual relocations, unless
-  // the InSet flag is set to get the current difference anyway (used for
-  // example to calculate symbol sizes).
-  if (Asm && canFold(Asm, LHS_A, LHS_B, InSet)) {
+  // If we have a layout, we can fold resolved differences.
+  if (Asm) {
     // First, fold out any differences which are fully resolved. By
     // reassociating terms in
     //   Result = (LHS_A - LHS_B + LHS_Cst) + (RHS_A - RHS_B + RHS_Cst).

llvm/lib/MC/MCObjectStreamer.cpp

 // As a compile-time optimization, avoid allocating and evaluating an MCExpr
 // tree for (Hi - Lo) when Hi and Lo are offsets into the same fragment.
 static Optional<uint64_t>
-absoluteSymbolDiff(MCAssembler &Asm, const MCSymbol *Hi, const MCSymbol *Lo) {
+absoluteSymbolDiff(const MCSymbol *Hi, const MCSymbol *Lo) {
   assert(Hi && Lo);
-  if (Asm.getBackendPtr()->requiresDiffExpressionRelocations())
-    return None;
-
   if (!Hi->getFragment() || Hi->getFragment() != Lo->getFragment() ||
       Hi->isVariable() || Lo->isVariable())
     return None;

 void MCObjectStreamer::emitAbsoluteSymbolDiff(const MCSymbol *Hi,
                                               const MCSymbol *Lo,
                                               unsigned Size) {
-  if (Optional<uint64_t> Diff = absoluteSymbolDiff(getAssembler(), Hi, Lo)) {
+  if (Optional<uint64_t> Diff = absoluteSymbolDiff(Hi, Lo)) {
     emitIntValue(*Diff, Size);
     return;
   }

 
 void MCObjectStreamer::emitAbsoluteSymbolDiffAsULEB128(const MCSymbol *Hi,
                                                        const MCSymbol *Lo) {
-  if (Optional<uint64_t> Diff = absoluteSymbolDiff(getAssembler(), Hi, Lo)) {
+  if (Optional<uint64_t> Diff = absoluteSymbolDiff(Hi, Lo)) {
     emitULEB128IntValue(*Diff);
     return;
   }

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

 
   void setForceRelocs() { ForceRelocs = true; }
 
-  // Returns true if relocations will be forced for shouldForceRelocation by
-  // default. This will be true if relaxation is enabled or had previously
-  // been enabled.
-  bool willForceRelocations() const {
-    return ForceRelocs || STI.getFeatureBits()[RISCV::FeatureRelax];
-  }
-
-  // Generate diff expression relocations if the relax feature is enabled or had
-  // previously been enabled, otherwise it is safe for the assembler to
-  // calculate these internally.
-  bool requiresDiffExpressionRelocations() const override {
-    return willForceRelocations();
-  }
-
   // Return Size with extra Nop Bytes for alignment directive in code section.
   bool shouldInsertExtraNopBytesForCodeAlign(const MCAlignFragment &AF,
                                              unsigned &Size) override;

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

       {"fixup_riscv_call", 0, 64, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_riscv_call_plt", 0, 64, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_riscv_relax", 0, 0, 0},
-      {"fixup_riscv_align", 0, 0, 0}};
+      {"fixup_riscv_align", 0, 0, 0},
+
+      {"fixup_riscv_add_8", 0, 0, 0},
+      {"fixup_riscv_sub_8", 0, 0, 0},
+
+      {"fixup_riscv_add_16", 0, 0, 0},
+      {"fixup_riscv_sub_16", 0, 0, 0},
+
+      {"fixup_riscv_add_32", 0, 0, 0},
+      {"fixup_riscv_sub_32", 0, 0, 0},
+
+      {"fixup_riscv_add_64", 0, 0, 0},
+      {"fixup_riscv_sub_64", 0, 0, 0},
+  };
   static_assert((array_lengthof(Infos)) == RISCV::NumTargetFixupKinds,
                 "Not all fixup kinds added to Infos array");
 

   case RISCV::fixup_riscv_tls_got_hi20:
   case RISCV::fixup_riscv_tls_gd_hi20:
     llvm_unreachable("Relocation should be unconditionally forced\n");
+  case RISCV::fixup_riscv_add_8:
+  case RISCV::fixup_riscv_sub_8:
+  case RISCV::fixup_riscv_add_16:
+  case RISCV::fixup_riscv_sub_16:
+  case RISCV::fixup_riscv_add_32:
+  case RISCV::fixup_riscv_sub_32:
+  case RISCV::fixup_riscv_add_64:
+  case RISCV::fixup_riscv_sub_64:
   case FK_Data_1:
   case FK_Data_2:
   case FK_Data_4:

   MCFixupKind Kind = Fixup.getKind();
   if (Kind >= FirstLiteralRelocationKind)
     return;
+
   MCContext &Ctx = Asm.getContext();
   MCFixupKindInfo Info = getFixupKindInfo(Kind);
+
   if (!Value)
     return; // Doesn't change encoding.
   // Apply any target-specific value adjustments.

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

 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/Support/ErrorHandling.h"
 
+#include <utility>
+
 using namespace llvm;
 
 namespace {

       return ELF::R_RISCV_CALL;
     case RISCV::fixup_riscv_call_plt:
       return ELF::R_RISCV_CALL_PLT;
+    case RISCV::fixup_riscv_add_8:
+      return ELF::R_RISCV_ADD8;
+    case RISCV::fixup_riscv_sub_8:
+      return ELF::R_RISCV_SUB8;
+    case RISCV::fixup_riscv_add_16:
+      return ELF::R_RISCV_ADD16;
+    case RISCV::fixup_riscv_sub_16:
+      return ELF::R_RISCV_SUB16;
+    case RISCV::fixup_riscv_add_32:
+      return ELF::R_RISCV_ADD32;
+    case RISCV::fixup_riscv_sub_32:
+      return ELF::R_RISCV_SUB32;
+    case RISCV::fixup_riscv_add_64:
+      return ELF::R_RISCV_ADD64;
+    case RISCV::fixup_riscv_sub_64:
+      return ELF::R_RISCV_SUB64;
     }
   }
 

     return ELF::R_RISCV_32;
   case FK_Data_8:
     return ELF::R_RISCV_64;
-  case FK_Data_Add_1:
-    return ELF::R_RISCV_ADD8;
-  case FK_Data_Add_2:
-    return ELF::R_RISCV_ADD16;
-  case FK_Data_Add_4:
-    return ELF::R_RISCV_ADD32;
-  case FK_Data_Add_8:
-    return ELF::R_RISCV_ADD64;
-  case FK_Data_Add_6b:
-    return ELF::R_RISCV_SET6;
-  case FK_Data_Sub_1:
-    return ELF::R_RISCV_SUB8;
-  case FK_Data_Sub_2:
-    return ELF::R_RISCV_SUB16;
-  case FK_Data_Sub_4:
-    return ELF::R_RISCV_SUB32;
-  case FK_Data_Sub_8:
-    return ELF::R_RISCV_SUB64;
-  case FK_Data_Sub_6b:
-    return ELF::R_RISCV_SUB6;
   case RISCV::fixup_riscv_hi20:
     return ELF::R_RISCV_HI20;
   case RISCV::fixup_riscv_lo12_i:

     return ELF::R_RISCV_RELAX;
   case RISCV::fixup_riscv_align:
     return ELF::R_RISCV_ALIGN;
+  case RISCV::fixup_riscv_add_8:
+    return ELF::R_RISCV_ADD8;
+  case RISCV::fixup_riscv_sub_8:
+    return ELF::R_RISCV_SUB8;
+  case RISCV::fixup_riscv_add_16:
+    return ELF::R_RISCV_ADD16;
+  case RISCV::fixup_riscv_sub_16:
+    return ELF::R_RISCV_SUB16;
+  case RISCV::fixup_riscv_add_32:
+    return ELF::R_RISCV_ADD32;
+  case RISCV::fixup_riscv_sub_32:
+    return ELF::R_RISCV_SUB32;
+  case RISCV::fixup_riscv_add_64:
+    return ELF::R_RISCV_ADD64;
+  case RISCV::fixup_riscv_sub_64:
+    return ELF::R_RISCV_SUB64;
   }
 }
 

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

   void emitDirectiveOptionRelax() override;
   void emitDirectiveOptionNoRelax() override;
 };
+
+MCELFStreamer *createRISCVELFStreamer(MCContext &C,
+                                      std::unique_ptr<MCAsmBackend> MAB,
+                                      std::unique_ptr<MCObjectWriter> MOW,
+                                      std::unique_ptr<MCCodeEmitter> MCE,
+                                      bool RelaxAll);
 }
 #endif

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

 #include "RISCVBaseInfo.h"
 #include "RISCVMCTargetDesc.h"
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/LEB128.h"

 void RISCVTargetELFStreamer::emitDirectiveOptionRVC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoRVC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionRelax() {
-  MCAssembler &MCA = getStreamer().getAssembler();
-  auto &MAB = static_cast<RISCVAsmBackend &>(MCA.getBackend());
-  MAB.setForceRelocs();
+  getStreamer().getAssembler().setRelax(true);
+}
+void RISCVTargetELFStreamer::emitDirectiveOptionNoRelax() {
+  getStreamer().getAssembler().setRelax(false);
 }
-void RISCVTargetELFStreamer::emitDirectiveOptionNoRelax() {}
 
 void RISCVTargetELFStreamer::emitAttribute(unsigned Attribute, unsigned Value) {
   setAttributeItem(Attribute, Value, /*OverwriteExisting=*/true);

   }
   return Result;
 }
+
+namespace {
+class RISCVELFStreamer : public MCELFStreamer {
+  static std::pair<unsigned, unsigned> getRelocPairForSize(unsigned Size) {
+    switch (Size) {
+    default: llvm_unreachable("unsupported fixup size");
+    case 1:
+      return std::make_pair(RISCV::fixup_riscv_add_8, RISCV::fixup_riscv_sub_8);
+    case 2:
+      return std::make_pair(RISCV::fixup_riscv_add_16,
+                            RISCV::fixup_riscv_sub_16);
+    case 4:
+      return std::make_pair(RISCV::fixup_riscv_add_32,
+                            RISCV::fixup_riscv_sub_32);
+    case 8:
+      return std::make_pair(RISCV::fixup_riscv_add_64,
+                            RISCV::fixup_riscv_sub_64);
+    }
+  }
+
+public:
+  RISCVELFStreamer(MCContext &C, std::unique_ptr<MCAsmBackend> MAB,
+                   std::unique_ptr<MCObjectWriter> MOW,
+                   std::unique_ptr<MCCodeEmitter> MCE)
+      : MCELFStreamer(C, std::move(MAB), std::move(MOW), std::move(MCE)) {}
+
+  void emitValueImpl(const MCExpr *Value, unsigned Size, SMLoc Loc) override {
+    const auto *MBE = dyn_cast<MCBinaryExpr>(Value);
+    if (!MBE)
+      return MCELFStreamer::emitValueImpl(Value, Size, Loc);
+
+    // Handle binary expressions manually so that we can create multiple fixups
+    // for the riscv_add_sub_n relocations.
+
+    MCStreamer::emitValueImpl(Value, Size, Loc);
+
+    MCDataFragment *DF = getOrCreateDataFragment();
+    flushPendingLabels(DF, DF->getContents().size());
+    MCDwarfLineEntry::make(this, getCurrentSectionOnly());
+
+    int64_t Absolute;
+    const auto *Asm = getAssemblerPtr();
+    if ((Asm ? Asm->getRelax() : false) &&
+        Value->evaluateAsAbsolute(Absolute, Asm)) {
+      if (!isUIntN(8 * Size, Absolute) && !isIntN(8 * Size, Absolute))
+        return getContext().reportError(
+            Loc, "value evaluated as " + Twine(Absolute) + " is out of range");
+      return emitIntValue(Absolute, Size);
+    }
+
+    // Create a pair of fixups for symbolic differences in text; if we find that
+    // relaxation is not enabled, we can drop the fixup during the finalization
+    // phase.
+    const auto *LHSSymRef = dyn_cast<MCSymbolRefExpr>(MBE->getLHS());
+    const auto *RHSSymRef = dyn_cast<MCSymbolRefExpr>(MBE->getRHS());
+    if (LHSSymRef) {
+      unsigned add, sub;
+      std::tie(add, sub) = getRelocPairForSize(Size);
+      DF->getFixups().push_back(
+          MCFixup::create(DF->getContents().size(), MBE->getLHS(),
+                          static_cast<MCFixupKind>(add), Loc));
+      DF->getFixups().push_back(
+          MCFixup::create(DF->getContents().size(), MBE->getRHS(),
+                          static_cast<MCFixupKind>(sub), Loc));
+    } else {
+      DF->getFixups().push_back(
+          MCFixup::create(DF->getContents().size(), Value,
+                          MCFixup::getKindForSize(Size, false), Loc));
+    }
+    DF->getContents().resize(DF->getContents().size() + Size, 0);
+  }
+};
+}
+
+namespace llvm {
+MCELFStreamer *createRISCVELFStreamer(MCContext &C,
+                                      std::unique_ptr<MCAsmBackend> MAB,
+                                      std::unique_ptr<MCObjectWriter> MOW,
+                                      std::unique_ptr<MCCodeEmitter> MCE,
+                                      bool RelaxAll) {
+  RISCVELFStreamer *S =
+      new RISCVELFStreamer(C, std::move(MAB), std::move(MOW), std::move(MCE));
+  S->getAssembler().setRelaxAll(RelaxAll);
+  return S;
+}
+}

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

   // relaxation.
   fixup_riscv_align,
 
+  fixup_riscv_add_8,
+  fixup_riscv_sub_8,
+
+  fixup_riscv_add_16,
+  fixup_riscv_sub_16,
+
+  fixup_riscv_add_32,
+  fixup_riscv_sub_32,
+
+  fixup_riscv_add_64,
+  fixup_riscv_sub_64,
+
   // fixup_riscv_invalid - used as a sentinel and a marker, must be last fixup
   fixup_riscv_invalid,
   NumTargetFixupKinds = fixup_riscv_invalid - FirstTargetFixupKind

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

 #include "RISCVTargetStreamer.h"
 #include "TargetInfo/RISCVTargetInfo.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCCodeEmitter.h"
 #include "llvm/MC/MCInstrAnalysis.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/ErrorHandling.h"

   return new RISCVMCInstrAnalysis(Info);
 }
 
+namespace {
+MCStreamer *createRISCVELFStreamer(const Triple &T, MCContext &Context,
+                                   std::unique_ptr<MCAsmBackend> &&MAB,
+                                   std::unique_ptr<MCObjectWriter> &&MOW,
+                                   std::unique_ptr<MCCodeEmitter> &&MCE,
+                                   bool RelaxAll) {
+  return createRISCVELFStreamer(Context, std::move(MAB), std::move(MOW),
+                                std::move(MCE), RelaxAll);
+}
+}
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTargetMC() {
   for (Target *T : {&getTheRISCV32Target(), &getTheRISCV64Target()}) {
     TargetRegistry::RegisterMCAsmInfo(*T, createRISCVMCAsmInfo);

     TargetRegistry::RegisterMCCodeEmitter(*T, createRISCVMCCodeEmitter);
     TargetRegistry::RegisterMCInstPrinter(*T, createRISCVMCInstPrinter);
     TargetRegistry::RegisterMCSubtargetInfo(*T, createRISCVMCSubtargetInfo);
+    TargetRegistry::RegisterELFStreamer(*T, createRISCVELFStreamer);
     TargetRegistry::RegisterObjectTargetStreamer(
         *T, createRISCVObjectTargetStreamer);
     TargetRegistry::RegisterMCInstrAnalysis(*T, createRISCVInstrAnalysis);

llvm/test/CodeGen/RISCV/fixups-relax-diff.ll

 ; RUN: llc -filetype=obj -mtriple=riscv32 -mattr=+relax %s -o - \
 ; RUN:     | llvm-readobj -r - | FileCheck -check-prefix=RELAX %s
 ; RUN: llc -filetype=obj -mtriple=riscv32 -mattr=-relax %s -o - \
-; RUN:     | llvm-readobj -r - | FileCheck -check-prefix=NORELAX %s
+; RUN:     | llvm-readobj -r - | FileCheck -check-prefix=RELAX %s
 
-; This test checks that a diff inserted via inline assembly only causes
-; relocations when relaxation is enabled. This isn't an assembly test
-; as the assembler takes a different path through LLVM, which is
-; already covered by the fixups-expr.s test.
+; This test checks that a diff inserted via inline assembly always causes
+; relocations. This isn't an assembly test as the assembler takes a different
+; path through LLVM, which is already covered by the fixups-expr.s test.
 
 define i32 @main() nounwind {
 entry:

   store i32 0, i32* %retval, align 4
   ; RELAX: R_RISCV_ADD64 b
   ; RELAX: R_RISCV_SUB64 a
-  ; NORELAX-NOT: R_RISCV_ADD
   call void asm sideeffect "a:\0Ab:\0A.dword b-a", ""()
   ret i32 0
 }

llvm/test/MC/RISCV/fixups-expr.s

 # RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+relax %s \
-# RUN:     | llvm-readobj -r - | FileCheck -check-prefix=RELAX %s
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX %s
 # RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=-relax %s \
-# RUN:     | llvm-readobj -r - | FileCheck -check-prefix=NORELAX %s
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX %s
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax %s \
-# RUN:     | llvm-readobj -r - | FileCheck -check-prefix=RELAX %s
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX %s
 # RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=-relax %s \
-# RUN:     | llvm-readobj -r - | FileCheck -check-prefix=NORELAX %s
+# RUN:     | llvm-readobj -r - | FileCheck -check-prefix RELAX %s
 
-# Check that subtraction expressions are emitted as two relocations
-# only when relaxation is enabled
+# Check that subtraction expressions are emitted as two relocations always.
 
 .globl G1
 .globl G2

 # RELAX: 0x1C R_RISCV_SUB8 .L1 0x0
 # RELAX: 0x1D R_RISCV_ADD8 G2 0x0
 # RELAX: 0x1D R_RISCV_SUB8 G1 0x0
-# NORELAX-NOT: R_RISCV