[RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook

[shiva0217]

Summary

For RISCV branch instructions, we need to preserve relocation types when linker relaxation enabled, so then linker could modify offset when the branch offsets changed.

If we preserve relocation types by define shouldForceRelocation, IsResolved will always false which will make RISCV MC Branch Relaxation always relax 16-bit branches to 32-bit form which is not we really want.

RISCV MC Branch Relaxation is needed because RISCV could perform 32-bit to 16-bit transformation in MC layer.

[sabuasal]

Hi Shiva,

Just a comment a

Quoted Text "if we define shouldForceRelocation for branches, fixup calculation will be disabled"

Implementing RISCVAsmBackend::shoulForceRelocation() will force "MCAssembler::evaluateFixup()" to return false for IsResolved which will cause the record of the relocation in "MCAssembler::handleFixup()" but the fixup value itself will still be computed.

I think the side effect that is causing relaxation to always be forced in other cases is because you don't have "fixupNeedsRelaxationAdvanced" overridden in RISCVAsmBackend. If you override it you can change the behavior of always having
the instruction relaxed if resolved is set to false. I was wondering if you took a look at implemeting the same thing you need by doing that!

[shiva0217]

In D44887#1048628, @sabuasal wrote:

Hi Shiva,

Just a comment a

Quoted Text "if we define shouldForceRelocation for branches, fixup calculation will be disabled"

Implementing RISCVAsmBackend::shoulForceRelocation() will force "MCAssembler::evaluateFixup()" to return false for IsResolved which will cause the record of the relocation in "MCAssembler::handleFixup()" but the fixup value itself will still be computed.

I think the side effect that is causing relaxation to always be forced in other cases is because you don't have "fixupNeedsRelaxationAdvanced" overridden in RISCVAsmBackend. If you override it you can change the behavior of always having
the instruction relaxed if resolved is set to false. I was wondering if you took a look at implemeting the same thing you need by doing that!

Hi Sameer. Thanks for your valuable input. I'll drop the patch and create https://reviews.llvm.org/D44971 to override "fixupNeedsRelaxationAdvanced".

[shiva0217]

We try to avoid RISCV MC Branch Relaxation always promote 16-bit branches to 32-bit form by override fixupNeedsRelaxationAdvanced. (https://reviews.llvm.org/D44971) It turns out that there's still some corner case we can't cover. So we would like to revive the patch.

[sabuasal]

test/MC/RISCV/rv32-relaxation.s

4

Can we add test to check that the relocation is actually recorded. same tests you had in https://reviews.llvm.org/D44971 make sense.

[shiva0217]

Update the test cases as @sabuasal 's comments.

[asb]

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

One option is we could just say that it's not worth doing exactly what gcc does here, and leave the linker to relax a 32-bit jump. Alternatively, I think the hooks could be structured differently. As I understand it: we want fixupNeedsRelaxation to return false in cases where evaluateFixup returns false, but does so because a relocation was forced. Achieve this by:

• Modifying evaluateFixup so it takes a bool ref ('WasForced' or similar) , which indicates when the fixup was evaluated, but is not fixed due to the return value of shouldForceRelocation
• Modify the evaluateFixup doc string to document the behaviour being relied upon: the Value in the fixup is set to the correct value if WasForced is true, even if evaluateFixup returns false
• Modify the signature of fixupNeedsRelaxationAdvanced so it passes along the WasForced flag
• Implement fixupNeedsRelaxationAdvanced in the riscvbackend, so it returns false if !Resolved && WasForced

Would the above resolve the issue? If so, the question is whether it's worth implementing this behaviour or not.

[sabuasal]

Hi Alex ,

In D44887#1068861, @asb wrote:

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

We have a test to check that compressed instructions to unresolved symbols in test/MC/RISCV/compressed-relocations.s.

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

One option is we could just say that it's not worth doing exactly what gcc does here, and leave the linker to relax a 32-bit jump. Alternatively, I think the hooks could be structured differently. As I understand it: we want fixupNeedsRelaxation to return false in cases where evaluateFixup returns false, but does so because a relocation was forced. Achieve this by:

• Modifying evaluateFixup so it takes a bool ref ('WasForced' or similar) , which indicates when the fixup was evaluated, but is not fixed due to the return value of shouldForceRelocation
• Modify the evaluateFixup doc string to document the behaviour being relied upon: the Value in the fixup is set to the correct value if WasForced is true, even if evaluateFixup returns false
• Modify the signature of fixupNeedsRelaxationAdvanced so it passes along the WasForced flag
• Implement fixupNeedsRelaxationAdvanced in the riscvbackend, so it returns false if !Resolved && WasForced

Would the above resolve the issue? If so, the question is whether it's worth implementing this behaviour or not.

I'll add one more suggestion that I mentioned on the other patch.

So we know that the the problem is that the relaxation logic, which happens during the layoutOnce() shares some logic with the Relocation recording by
calling evaluateFixup which checks shouldForceRelocation in the backend. The framework gives the ability to decouple the two behaviors by implenting RISCVMCBackend::fixupNeedsRelaxationAdvanced, so that when it
called from MCAssembler::fixupNeedsRelaxation() (during the layoutOnce() which does relaxation) the backend can implemet whatever it prefers.
To do that without unintentionally affecting other back ends we can implement the logic of evaluateFixup() in fixupNeedsRelaxationAdvanced() and just ignore what evaluate fixup pases for "Resolved;" and value.

Another suggestion that requires changing the base classes, move the lines:

>   if (IsResolved && Backend.shouldForceRelocation(*this, Fixup, Target))
>     IsResolved = false;
>

from evaluate fixup and do the check for "shouldforceRelocaton()" in MCAssembler::handleFixup().

[shiva0217]

In D44887#1068861, @asb wrote:

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

Hi Alex.
In fact, the value is fixed as docstring description. You could observe the test case rv32-relaxation.s. When the relaxation enables, the branch operands will fill with fixup value but also preserve the relocation types and that's the target hook shouldForceRelocationWithApplyFixup try to achieve.

One option is we could just say that it's not worth doing exactly what gcc does here, and leave the linker to relax a 32-bit jump.

Currently, Binutils LD not support relax c.jal to jal. It relaxes to jal by Binutils AS.
Relaxing c.jal to jal for an unresolved symbol seems makes sense but I hope we won't add too much complexity.

Alternatively, I think the hooks could be structured differently. As I understand it: we want fixupNeedsRelaxation to return false in cases where evaluateFixup returns false, but does so because a relocation was forced. Achieve this by:

• Modifying evaluateFixup so it takes a bool ref ('WasForced' or similar) , which indicates when the fixup was evaluated, but is not fixed due to the return value of shouldForceRelocation

Something like if (IsResolved && Backend.shouldForceRelocation(*this, Fixup, Target)) WasForced = true; right?

• Modify the evaluateFixup doc string to document the behaviour being relied upon: the Value in the fixup is set to the correct value if WasForced is true, even if evaluateFixup returns false
• Modify the signature of fixupNeedsRelaxationAdvanced so it passes along the WasForced flag
• Implement fixupNeedsRelaxationAdvanced in the riscvbackend, so it returns false if !Resolved && WasForced

I think it should be if (!Resolved && WasForced) return fixupNeedsRelaxation(Fixup, Value, DF, Layout);

Would the above resolve the issue? If so, the question is whether it's worth implementing this behaviour or not.

Yes, it seems workable, but it would need to modify more general code than introduce the new target hook.

[asb]

In D44887#1069573, @shiva0217 wrote:

In D44887#1068861, @asb wrote:

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

Hi Alex.
In fact, the value is fixed as docstring description. You could observe the test case rv32-relaxation.s. When the relaxation enables, the branch operands will fill with fixup value but also preserve the relocation types and that's the target hook shouldForceRelocationWithApplyFixup try to achieve.

I suppose it depends on the interpretation of the docstring. It currently reads:

/// \return Whether the fixup value was fully resolved. This is true if the
/// \p Value result is fixed, otherwise the value may change due to
/// relocation.

Although a Value has been generated, it certainly may change due to relocation.

One option is we could just say that it's not worth doing exactly what gcc does here, and leave the linker to relax a 32-bit jump.

Currently, Binutils LD not support relax c.jal to jal. It relaxes to jal by Binutils AS.

Thanks, good to know. That's definitely a strong motivation for matching gas behaviour then.

Relaxing c.jal to jal for an unresolved symbol seems makes sense but I hope we won't add too much complexity.

Alternatively, I think the hooks could be structured differently. As I understand it: we want fixupNeedsRelaxation to return false in cases where evaluateFixup returns false, but does so because a relocation was forced. Achieve this by:

• Modifying evaluateFixup so it takes a bool ref ('WasForced' or similar) , which indicates when the fixup was evaluated, but is not fixed due to the return value of shouldForceRelocation

Something like if (IsResolved && Backend.shouldForceRelocation(*this, Fixup, Target)) WasForced = true; right?

Yes.

• Modify the evaluateFixup doc string to document the behaviour being relied upon: the Value in the fixup is set to the correct value if WasForced is true, even if evaluateFixup returns false
• Modify the signature of fixupNeedsRelaxationAdvanced so it passes along the WasForced flag
• Implement fixupNeedsRelaxationAdvanced in the riscvbackend, so it returns false if !Resolved && WasForced

I think it should be if (!Resolved && WasForced) return fixupNeedsRelaxation(Fixup, Value, DF, Layout);

I don't think we want a call from fixupNeedsRelaxationAdvanced -> fixupNeedsRelaxation.

Would the above resolve the issue? If so, the question is whether it's worth implementing this behaviour or not.

Yes, it seems workable, but it would need to modify more general code than introduce the new target hook.

It does modify some more general code, but it has minimal impact. evaluateFixup has only two callsites, fixupNeedsRelaxationAdvanced is used only by Hexagon, and presumably added to avoid modifying the signature of fixupNeedsRelaxation for everybody. Adding a third similar hook would seem unfortunate when it's relatively easy to modify fixupNeedsRelaxationAdvanced.

[shiva0217]

In D44887#1069614, @asb wrote:

In D44887#1069573, @shiva0217 wrote:

In D44887#1068861, @asb wrote:

Could you please expand rv32-relaxation.s and rv64-relaxation.s so the test includes the behaviour for branches to an unresolved symbol?

I'm a bit concerned about adding shouldForceRelocationWithApplyFixup. It adds complexity by adding another way of achieving something very similar to the existing shouldForceRelocation, and its use means that the return value of evaluateFixup no longer matches the description in its docstring (the value isn't 'fixed' by the definition in the docstring, but evaluateFixup still returns true).

Hi Alex.
In fact, the value is fixed as docstring description. You could observe the test case rv32-relaxation.s. When the relaxation enables, the branch operands will fill with fixup value but also preserve the relocation types and that's the target hook shouldForceRelocationWithApplyFixup try to achieve.

I suppose it depends on the interpretation of the docstring. It currently reads:

/// \return Whether the fixup value was fully resolved. This is true if the
/// \p Value result is fixed, otherwise the value may change due to
/// relocation.

Although a Value has been generated, it certainly may change due to relocation.

One option is we could just say that it's not worth doing exactly what gcc does here, and leave the linker to relax a 32-bit jump.

Currently, Binutils LD not support relax c.jal to jal. It relaxes to jal by Binutils AS.

Thanks, good to know. That's definitely a strong motivation for matching gas behaviour then.

Relaxing c.jal to jal for an unresolved symbol seems makes sense but I hope we won't add too much complexity.

Alternatively, I think the hooks could be structured differently. As I understand it: we want fixupNeedsRelaxation to return false in cases where evaluateFixup returns false, but does so because a relocation was forced. Achieve this by:

• Modifying evaluateFixup so it takes a bool ref ('WasForced' or similar) , which indicates when the fixup was evaluated, but is not fixed due to the return value of shouldForceRelocation

Something like if (IsResolved && Backend.shouldForceRelocation(*this, Fixup, Target)) WasForced = true; right?

Yes.

• Modify the evaluateFixup doc string to document the behaviour being relied upon: the Value in the fixup is set to the correct value if WasForced is true, even if evaluateFixup returns false
• Modify the signature of fixupNeedsRelaxationAdvanced so it passes along the WasForced flag
• Implement fixupNeedsRelaxationAdvanced in the riscvbackend, so it returns false if !Resolved && WasForced

I think it should be if (!Resolved && WasForced) return fixupNeedsRelaxation(Fixup, Value, DF, Layout);

I don't think we want a call from fixupNeedsRelaxationAdvanced -> fixupNeedsRelaxation.

I thought we will need call fixupNeedsRelaxation, because WasForced is true means the symbol could be resovled, but the offset could fit in or not would need fixupNeedsRelaxation to identify.
Is that reasonable?

Would the above resolve the issue? If so, the question is whether it's worth implementing this behaviour or not.

Yes, it seems workable, but it would need to modify more general code than introduce the new target hook.

It does modify some more general code, but it has minimal impact. evaluateFixup has only two callsites, fixupNeedsRelaxationAdvanced is used only by Hexagon, and presumably added to avoid modifying the signature of fixupNeedsRelaxation for everybody. Adding a third similar hook would seem unfortunate when it's relatively easy to modify fixupNeedsRelaxationAdvanced.

I suppose introducing the new target hook would not be an option. Sadly, I would drop the patch.

[asb]

In D44887#1069639, @shiva0217 wrote:

In D44887#1069614, @asb wrote:

In D44887#1069573, @shiva0217 wrote:

I think it should be if (!Resolved && WasForced) return fixupNeedsRelaxation(Fixup, Value, DF, Layout);

I don't think we want a call from fixupNeedsRelaxationAdvanced -> fixupNeedsRelaxation.

I thought we will need call fixupNeedsRelaxation, because WasForced is true means the symbol could be resovled, but the offset could fit in or not would need fixupNeedsRelaxation to identify.
Is that reasonable?

Hexagon at least just has fixupNeedsRelaxation assert, as all logic is in one place in fixupNeedsRelaxationAdvanced. That seems a sensible way of doing things.

[shiva0217]

In D44887#1069641, @asb wrote:

In D44887#1069639, @shiva0217 wrote:

In D44887#1069614, @asb wrote:

In D44887#1069573, @shiva0217 wrote:

I think it should be if (!Resolved && WasForced) return fixupNeedsRelaxation(Fixup, Value, DF, Layout);

I don't think we want a call from fixupNeedsRelaxationAdvanced -> fixupNeedsRelaxation.

I thought we will need call fixupNeedsRelaxation, because WasForced is true means the symbol could be resovled, but the offset could fit in or not would need fixupNeedsRelaxation to identify.
Is that reasonable?

Hexagon at least just has fixupNeedsRelaxation assert, as all logic is in one place in fixupNeedsRelaxationAdvanced. That seems a sensible way of doing things.

OK, that makes sense.

[shiva0217]

The new revision https://reviews.llvm.org/D46350 has been created 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;
   }
 
+  /// Same as shouldForceRelocation but also apply Fixup.
+  virtual bool shouldForceRelocationWithApplyFixup(const MCAssembler &Asm,
+                                                   const MCFixup &Fixup,
+                                                   const MCValue &Target) {
+    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

MCAssembler::handleFixup(const MCAsmLayout &Layout, MCFragment &F,
   MCValue Target;
   uint64_t FixedValue;
   bool IsResolved = evaluateFixup(Layout, Fixup, &F, Target, FixedValue);
   if (!IsResolved) {
     // 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.
     getWriter().recordRelocation(*this, Layout, &F, Fixup, Target, FixedValue);
+  } else if (Backend.shouldForceRelocationWithApplyFixup(*this, Fixup, Target)) {
+    // For RISCV branch instructions, we need to insert relocation types, so
+    // that linker relaxation could modify offset. But also calculate fixup
+    // value, because RISCV MC Branch Relaxation depend on it.
+    // RISCV MC Branch Relaxation is needed because RISCV could perform 32-bit
+    // to 16-bit transformation in MC layer.
+    uint64_t Value = 0;
+    getWriter().recordRelocation(*this, Layout, &F, Fixup, Target, Value);
   }
   return std::make_tuple(Target, FixedValue, IsResolved);
 }
 
 void MCAssembler::layout(MCAsmLayout &Layout) {
   DEBUG_WITH_TYPE("mc-dump", {
       errs() << "assembler backend - pre-layout\n--\n";
       dump(); });

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
     //
     // For R_RISCV_PCREL_LO12_I, linker will
     // 1. Find high part instruction(auipc) by .Ltmp3
     // 2. Find symbol(foo) in auipc's relocation type
     // 3. Resolve offset by symbol(foo)
     if (FixupKind == RISCV::fixup_riscv_pcrel_lo12_i ||
         FixupKind == RISCV::fixup_riscv_pcrel_hi20)
       return true;
+    return false;
+  }
+
+  // To apply fixup calculation with preserve relocation types.
+  // Because we need assembler support branch relaxation for MC 32-bit to
+  // 16-bit instruction transformation. MC branch offset will depend on
+  // fixup calculation. If we define shouldForceRelocation for branches,
+  // fixup calculation will be disabled.
+  // We also need preserve relocation types becuase linker relaxation could
+  // modify branch offset when the instructions between branch and branch target
+  // have been relaxed. We need preserve branch relocation type for linker to
+  // correct the offset.
+  bool shouldForceRelocationWithApplyFixup(const MCAssembler &Asm,
+                                           const MCFixup &Fixup,
+                                           const MCValue &Target) 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 {

test/MC/RISCV/rv32-relaxation.s

 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
 # RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c,+relax < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
 
 FAR_JUMP_NEGATIVE:
   c.nop
 .space 2000
 
 FAR_BRANCH_NEGATIVE:
   c.nop
 .space 256

test/MC/RISCV/rv64-relaxation.s

 # RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c < %s \
 # RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c,+relax < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
 
 FAR_JUMP_NEGATIVE:
   c.nop
 .space 2000
 
 FAR_BRANCH_NEGATIVE:
   c.nop
 .space 256