Skip to content

LLD does not support the relax CPU feature of RISC-V (R_RISCV_ALIGN) #3451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LemonBoy opened this issue Oct 13, 2019 · 10 comments
Open

LLD does not support the relax CPU feature of RISC-V (R_RISCV_ALIGN) #3451

LemonBoy opened this issue Oct 13, 2019 · 10 comments
Labels
arch-riscv 32-bit and 64-bit RISC-V contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. linking
Milestone

Comments

@LemonBoy
Copy link
Contributor

It seems that the RISC-V assembler is somehow emitting the wrong amount of nops when the relax feature is active.

Here's a minimal test case:

define void @foo() unnamed_addr align 4 {
Entry:
    ret void
}
define void @bar() unnamed_addr align 16 {
Entry:
    ret void
}

When compiled with

llc-10 -mattr=+c,+relax -mtriple=riscv64 -filetype obj -o /tmp/foo.o /tmp/foo.ll

Produces the following

0000000000000000 .text:
       0: 01 00                        	nop
       2: 01 00                        	nop

0000000000000004 foo:
       4: 82 80                        	ret
       6: 13 00 00 00                  	nop
       a: 13 00 00 00                  	nop
       e: 13 00 00 00                  	nop
      12: 01 00                        	nop

0000000000000014 bar:
      14: 82 80                        	ret

This should be reported to the LLVM developers.

CC @luismarques

@luismarques
Copy link

luismarques commented Oct 13, 2019

If you do an objdump -dr you'll see that the those extra nops have a R_RISCV_ALIGN relocation (not all of the nops need to have the relocation, as long as they are sequential). In general, the "extra" nops are needed because the linker code relaxations can change the code alignment. The linker will remove extraneous nops from the sequence marked with R_RISCV_ALIGN when producing the final ELF program, keeping only the necessary ones to meet the requested alignment. Now, it's true that we could not emit some of those nops in the first place, even if emitting them is not a correctness issue. By coincidence I worked a bit on that last week, but it turns out that cleanly solving that issue is not quite trivial with the current LLVM implementation, and I'm not sure it's worth spending more time on that, given that there's no correctness problem.

@LemonBoy
Copy link
Contributor Author

The linker will remove extraneous nops from the sequence marked with R_RISCV_ALIGN when producing the final ELF program, keeping only the necessary ones to meet the requested alignment.

It seems that lld is keeping all of them and the R_RISCV_ALIGN relocation is gone:

Disassembly of section .text:

0000000000011170 .text:
   11170: 01 00                        	nop
   11172: 01 00                        	nop

0000000000011174 foo:
   11174: 82 80                        	ret
   11176: 13 00 00 00                  	nop
   1117a: 13 00 00 00                  	nop
   1117e: 13 00 00 00                  	nop
   11182: 01 00                        	nop

0000000000011184 bar:
   11184: 82 80                        	ret

@luismarques
Copy link

lld currently doesn't support code relaxations, so that's to be expected. If you want to enable code relaxations you'll have to use the gnu linker at the moment.

@andrewrk
Copy link
Member

What does the relax feature do? Maybe we could disable it for the default target settings.

@luismarques
Copy link

That's certainly possible. The idea of code relaxations is that you replace longer instruction sequences with shorter, more optimized ones depending on information that is only available at link time. For instance, in RISC-V you can add a 12-bit immediate to a register in a single instruction, but it takes two instructions to add a 32-bit immediate. If the immediate comes from a symbol for which the final value is only known by the linker you must emit the two-instruction sequence. Yet, you can annotate the sequence with a R_RISCV_RELAX relocation to allow the linker to replace them with the single instruction, if the immediate value turns out to fit the 12-bit immediate.

@luismarques
Copy link

The reason for the weird nops is that when you have compressed instructions enabled the linker might generate 16-bit instructions when doing the code relaxations, affecting the alignment.

@andrewrk
Copy link
Member

Ok, given that this is an optimization, I think it is reasonable to disable it until we have linker support for it, whether by improvements to LLD or #1535.

@andrewrk andrewrk added this to the 0.6.0 milestone Oct 14, 2019
@andrewrk andrewrk added arch-riscv 32-bit and 64-bit RISC-V bug Observed behavior contradicts documented or intended behavior upstream An issue with a third party project that Zig uses. labels Oct 14, 2019
@andrewrk andrewrk changed the title LLVM inserts too many alignment bytes on RISC-V LLD does not support the relax CPU feature of RISC-V (R_RISCV_ALIGN) Feb 18, 2020
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. and removed bug Observed behavior contradicts documented or intended behavior labels Feb 18, 2020
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Feb 18, 2020
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 13, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Nov 6, 2020
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 May 19, 2021
@alexrp
Copy link
Member

alexrp commented Sep 5, 2024

FWIW, LLD has long since gained support for various kinds of relaxations.

@alexrp
Copy link
Member

alexrp commented Oct 2, 2024

@Rexicon226 you're working on zld support for RISC-V relaxations, right?

@Rexicon226
Copy link
Contributor

@Rexicon226 you're working on zld support for RISC-V relaxations, right?

Yes, I would like to implement this in self-hosted before closing the issue.

@alexrp alexrp added linking and removed upstream An issue with a third party project that Zig uses. labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-riscv 32-bit and 64-bit RISC-V contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. linking
Projects
None yet
Development

No branches or pull requests

5 participants