This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook
AbandonedPublic

Authored by shiva0217 on Mar 25 2018, 6:34 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Mar 25 2018, 6:34 PM

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 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 abandoned this revision.Mar 27 2018, 10:44 PM
shiva0217 edited the summary of this revision. (Show Details)Apr 9 2018, 2:30 AM
shiva0217 retitled this revision from [RISCV] Add shouldForceRelocationWithApplyFixup Target Hook to calculate fixup value but also leave relocation types to [RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook.
shiva0217 edited the summary of this revision. (Show Details)

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 added inline comments.Apr 15 2018, 9:52 PM
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.

Update the test cases as @sabuasal 's comments.

asb added a comment.Apr 16 2018, 9:21 AM

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.

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().

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 added a comment.Apr 17 2018, 1:19 AM
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.

In D44887#1069614, @asb 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 added a comment.Apr 17 2018, 2:10 AM
In D44887#1069614, @asb 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.

In D44887#1069641, @asb wrote:
In D44887#1069614, @asb 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 abandoned this revision.May 1 2018, 10:41 PM

The new revision https://reviews.llvm.org/D46350 has been created to replace this one.