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

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added a comment.

Hi Alex ,

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?


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


Repository:
  rL LLVM

https://reviews.llvm.org/D44887