[PATCH] D103539: WIP: try to repair RISCV handling of paired relocations

Fri Jun 4 15:16:15 PDT 2021

compnerd added inline comments.

================
Comment at: llvm/test/CodeGen/RISCV/fixups-relax-diff.ll:19
   call void asm sideeffect "a:\0Ab:\0A.dword b-a", ""()
   ret i32 0
 }
----------------
jrtc27 wrote:
> There's no longer a RUN line for this... also this test makes no sense in the first place, it's trying to use 64-bit relocations on RV32. That should just be an error.
I didn't even notice that the relocation types are just flat out wrong.  That seems like a separate issue.

I wanted to keep the now dead check to do a last round of verification of the changes, at which point it should be removed.

================
Comment at: llvm/test/MC/RISCV/rv64-relax-all.s:12
 # RELAX-INSTR:     jal    zero, 0x0 <NEAR>
 c.j NEAR
----------------
jrtc27 wrote:
> Again, no longer used by a RUN line. And this looks like a bug, we're passing --mc-relax-all to the assembler, so this branch should have been relaxed in the branch relaxation sense (which is not the same as the linker relaxation sense), no? GNU as doesn't have --mc-relax-all, comparing against it is meaningless.
Similar to above, I wanted to keep it until I do the final sweep through the change.

Agreed, that matching the behaviour with GAS for this does not make sense.  I will have to adjust the implementation to accommodate this once I figure out the remaining cases.

It does raise the question of does it make sense for us to have a `-mrelax` and a `-mc-relax-all`?  Perhaps we should just treat `-mrelax` as `-mc-relax-all`.  Is there a heavy penalty for this?  Yes, linking may take a little bit more time and memory and the object file size will be slightly larger, but there shouldn't be any runtime cost.  The question boils down to, is the slight link time savings worth the two separate paths.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103539/new/

https://reviews.llvm.org/D103539