Skip to content
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

Upgrade rust toolchain to nightly-2024-11-05 #585

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Rahix
Copy link
Owner

@Rahix Rahix commented Sep 25, 2024

Time for another toolchain upgrade to fix a number of known compiler bugs.

@Rahix
Copy link
Owner Author

Rahix commented Sep 25, 2024

@Patryk27, some CI jobs are failing with

out of range branch target (expected an integer in the range -4096 to 4095)

is this still one of the known problems or something new?

@Patryk27
Copy link
Contributor

This is known, it’s already fixed in LLVM, but not yet backported - it should be in rustc within a couple days hopefully; I’ll let you know, so we can wait before bumping the toolchain here to get all of the fixes 🙂

@Rahix Rahix changed the title Upgrade rust toolchain to nightly-2024-09-24 Upgrade rust toolchain to nightly-2024-10-01 Oct 2, 2024
@Patryk27
Copy link
Contributor

@Rahix I think we should be good now - could you bump to the newest toolchain? 👀

@Rahix Rahix changed the title Upgrade rust toolchain to nightly-2024-10-01 Upgrade rust toolchain to nightly-2024-10-11 Oct 12, 2024
@Rahix
Copy link
Owner Author

Rahix commented Oct 12, 2024

@Patryk27, unfortunately it looks like we are still hitting the dreaded error: out of range branch target with nightly-2024-10-11? 😞

Just to check, I also tried nightly-2024-10-12, but I can still reproduce the error message...

@Patryk27
Copy link
Contributor

Patryk27 commented Oct 12, 2024

Ouch, my fault! It seems that llvm/llvm-project#109124 wasn't backported - I'll handle it tomorrow. Other commits seem to be in place, though (e.g. #573 is fixed), so we're mostly ready 😄

@Rahix
Copy link
Owner Author

Rahix commented Oct 13, 2024

No worries! Just ping me once the patch has hit nightly Rust, then I'll give it another shot :)

Thanks for all your compiler work!

@Patryk27
Copy link
Contributor

Patryk27 commented Nov 4, 2024

Alright, let's try now - the appropriate fix should've gone with rust-lang/rust#132352.

@Rahix Rahix changed the title Upgrade rust toolchain to nightly-2024-10-11 Upgrade rust toolchain to nightly-2024-11-03 Nov 4, 2024
@Rahix
Copy link
Owner Author

Rahix commented Nov 4, 2024

Updated to nightly-2024-11-03, but CI still reports the error: out of range branch target for some targets. Disabled fail-fast so we get the full list of failing targets - I checked, they are all failing on the same error.

Is the fix maybe not actually in this version I tried?

@Patryk27
Copy link
Contributor

Patryk27 commented Nov 5, 2024

Could you try 2024-11-05?
The fix is there, but since it got merged on 2024-11-02, the 2024-11-03 build might not contain it just yet 👀

Either that or it's going to be a new kind of error, we'll see 😄

Rahix added 2 commits November 5, 2024 18:28
This allows us to see which targets are currently failing the build.
@Rahix Rahix changed the title Upgrade rust toolchain to nightly-2024-11-03 Upgrade rust toolchain to nightly-2024-11-05 Nov 5, 2024
@Rahix
Copy link
Owner Author

Rahix commented Nov 5, 2024

Updated to 2024-11-05, still same targets failing... So I guess this is a new thing then? 😅

@Patryk27
Copy link
Contributor

Patryk27 commented Nov 6, 2024

Looks this way 😅 I'll try reproducing locally and report back.

@Patryk27
Copy link
Contributor

Patryk27 commented Nov 15, 2024

Status: reproduced!

$ cd ~/avr-hal/mcu/attiny-hal
$ RUSTFLAGS='--emit=llvm-ir' cargo +stage1 build --features attiny85 -Z build-std=core --target ../../avr-specs/avr-attiny85.json --release
$ llc -march=avr -mcpu=attiny85 -filetype=obj ~/avr-hal/target/avr-attiny85/release/deps/core-*.ll

... will crash, saying out of range branch target.

I've altered the error message in LLVM to print the offset it's trying to generate instruction for and it's pretty obvious something's off:

<unknown>:0: error: out of range branch target (expected an integer in the range -4096 to 4095, got 4488)
/* ... */
<unknown>:0: error: out of range branch target (expected an integer in the range -4096 to 4095, got 102544)
/* ... */
<unknown>:0: error: out of range branch target (expected an integer in the range -4096 to 4095, got 18446744073709481512)
/* ... */

I'll try narrowing the issue down 🫡

@Patryk27
Copy link
Contributor

Got it!

Reduced:
https://gist.github.com/Patryk27/a68ed01eeb57b59c3025c395059f906d
(and then llc -march=avr -mcpu=attiny85 -filetype=obj core.reduced.ll)

The issue boils down to too large jumps - basically, given:

foo:
  rjmp bar

# lots of code

bar:
  # something

... if there's too many opcodes between rjmp and the target label, we might not have enough space in the opcode's encoding to represent the offset (rjmp allows for 12 bits, for instance).

To avoid this issue, LLVM has a pass called branch relaxation that detects too large jumps and upgrades them into different instructions - in the case of AVR, that's where the backend might decide to emit jmp instead; it's one clock cycle more, but allows for a larger offset.

Everything's fine if the target AVR supports jmp - attiny85 does not, so the backend decides to stick with rjmp:

https://github.com/llvm/llvm-project/blob/a76609dd72743c0d678504042b00d434e6cebe1a/llvm/lib/Target/AVR/AVRInstrInfo.cpp#L568

... just to crash a moment later, in sort of a ¯_(ツ)_/¯ whaddya-want-me-to-do fashion.

Fortunately, not all hope is lost! - there's two ways out of this situation:

  • we can exploit the fact that AVR memory wraps, so instead of generating e.g. rjmp +5000, we can emit rjmp -3192 (cursed, but practical),
  • we can emit ijmp (this would be a rather heavy approach, though, since we'd have to store the Z register somewhere, update it, do the jump and then restore the register at the target site).

We didn't have to worry about those offsets, because in the past - up until llvm/llvm-project@6859685 aka llvm/llvm-project#106722 - LLVM handled jumps by relying on relocations. Relocations allow to store larger offsets with the assumption that "the linker will somehow deal with them later", and it did: (most likely) by applying the wrapping trick.

tl;dr linker smart, linker can solve "large jumps" issue - but llvm no longer rely on linker for jump, llvm must be taught to emit long jumps on its own

I'll try preparing the patch in the upcoming days.

@Rahix
Copy link
Owner Author

Rahix commented Nov 21, 2024

Damn, nice work!

we can exploit the fact that AVR memory wraps, so instead of generating e.g. rjmp +5000, we can emit rjmp -3192 (cursed, but practical)

haha, i love it 😆 cursed, indeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants