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

Nightly stops to accept registers r8-15 on thumbv6m #99071

Closed
jannic opened this issue Jul 8, 2022 · 8 comments · Fixed by #99155
Closed

Nightly stops to accept registers r8-15 on thumbv6m #99071

jannic opened this issue Jul 8, 2022 · 8 comments · Fixed by #99155
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jannic
Copy link
Contributor

jannic commented Jul 8, 2022

Code

I tried this code: (with --target thumbv6m-none-eabi)

#![no_std]

pub fn test() {
    unsafe {
        core::arch::asm!(
            "nop",
            in("r8") 1,
        );
    }
}

I expected to see this happen: Code compiles, resulting code should be something like:

example::test:
        push    {r6, r7, lr}
        add     r7, sp, #4
        mov     lr, r8
        push    {lr}
        movs    r0, #1
        mov     r8, r0
        nop
        pop     {r0}
        mov     r8, r0
        pop     {r6, r7, pc}

https://godbolt.org/z/c4Y49qz7q

Instead, this happened: Compilation fails with error message:

error: cannot use register `r8`: high registers (r8+) can only be used as clobbers in Thumb-1 code
 --> src/lib.rs:7:13
  |
7 |             in("r8") 1,
  |             ^^^^^^^^^^

https://godbolt.org/z/b6cnG4nM3

Version it worked on

It works on both 1.62.0 and 1.63.0-beta.4, and all other stable versions I tried since asm! got stabilized.

@Dirbaio did some bisecting and found: nightly-2021-12-09 works, nightly-2021-12-10 fails

Version with regression

I tried it with:
rustc --version --verbose:

rustc 1.64.0-nightly (1517f5de0 2022-07-07)
binary: rustc
commit-hash: 1517f5de01c445b5124b30f02257b02b4c5ef3b2
commit-date: 2022-07-07
host: x86_64-unknown-linux-gnu
release: 1.64.0-nightly
LLVM version: 14.0.6

I don't have details about the versions @Dirbaio tried.

@jannic jannic added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jul 8, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 8, 2022
@jannic
Copy link
Contributor Author

jannic commented Jul 9, 2022

Related:

But both commits should be part of current stable, so those don't explain the regression.

@Amanieu
Copy link
Member

Amanieu commented Jul 9, 2022

This was an intentional change to fix asm! on Thumb1 targets (including thumbv6m since it doesn't support most of the 32-bit thumb instructions). If you need to use the high registers then you should set their value yourself and mark them as clobbered.

@Dirbaio
Copy link
Contributor

Dirbaio commented Jul 9, 2022

@Amanieu The intended behavior is it should be rejected? It is accepted in stable even though it's been rejected in nightly for months. Perhaps the bug is it's accepted in stable when it shouldn't be?

@Amanieu
Copy link
Member

Amanieu commented Jul 9, 2022

Yes, this should be rejected. It is strange that this is being accepted on beta/stable, I'm not sure what could possibly be causing that...

@Amanieu
Copy link
Member

Amanieu commented Jul 9, 2022

OK I see what the problem is: sess.target_features only includes unstable target features on nightly builds, and the condition for the high Thumb registers only triggers when the unstable thumb_mode target feature is enabled.

@jannic
Copy link
Contributor Author

jannic commented Jul 9, 2022

BTW, the current stable behavior looks very sensible: Only registers r0-6 (r7 is used as the frame pointer) are assigned automatically when specifying the register class reg, so one can be sure that an assigned register can be used with every assembly instruction, even on cortex-m0 where the high registers can only be used on a small subset of instructions.
But if the user needs more registers, it is possible to specify the high registers manually.

So code like this (contrived) example is possible:

pub fn needs_many_args() {
    unsafe {
        core::arch::asm!(
            "mov {a0}, {a0}",
            "mov {a1}, {a1}",
            "mov {a2}, {a2}",
            "mov {a3}, {a3}",
            "mov {a4}, {a4}",
            "mov {a5}, {a5}",
            "mov {a6}, {a6}",
            "mov r8, r8",
            a0 = in(reg) 0,
            a1 = in(reg) 1,
            a2 = in(reg) 2,
            a3 = in(reg) 3,
            a4 = in(reg) 4,
            a5 = in(reg) 5,
            a6 = in(reg) 6,
            in("r8") 7,
        );
    }
}

https://godbolt.org/z/qqeKcvKz3

This can't easily be done on nightly.

Or am I missing something and what stable does is somehow broken?

I guess in practice it doesn't matter much and it's usually possible to get by with less arguments.

@Amanieu
Copy link
Member

Amanieu commented Jul 11, 2022

Or am I missing something and what stable does is somehow broken?

I'm working on a fallback register allocator when the backend can't support inline assembly. The problem is that the reg register class is currently defined to include all registers r0-r14 so high registers may be allocated for the reg register class.

It might be possible to have a separate register class in the future which allows high registers on Thumb1 targets though.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium T-compiler

@rustbot rustbot added P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 13, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jul 13, 2022
…=davidtwco

Keep unstable target features for asm feature checking

Inline assembly uses the target features to determine which registers
are available on the current target. However it needs to be able to
access unstable target features for this.

Fixes rust-lang#99071
@bors bors closed this as completed in e51f1b7 Jul 13, 2022
JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Jul 26, 2022
Inline assembly uses the target features to determine which registers
are available on the current target. However it needs to be able to
access unstable target features for this.

Fixes rust-lang#99071
JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Jul 26, 2022
…=davidtwco

Keep unstable target features for asm feature checking

Inline assembly uses the target features to determine which registers
are available on the current target. However it needs to be able to
access unstable target features for this.

Fixes rust-lang#99071
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Mar 7, 2023
Inline assembly uses the target features to determine which registers
are available on the current target. However it needs to be able to
access unstable target features for this.

Fixes rust-lang#99071
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this issue Mar 7, 2023
…=davidtwco

Keep unstable target features for asm feature checking

Inline assembly uses the target features to determine which registers
are available on the current target. However it needs to be able to
access unstable target features for this.

Fixes rust-lang#99071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-untriaged Untriaged performance or correctness regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants