[PATCH] D44887: [RISCV] Add shouldForceRelocationWithApplyFixup MC AsmBackend Target Hook

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In https://reviews.llvm.org/D44887#1069573, @shiva0217 wrote:

> In https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D44887