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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added a comment.

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.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D44887