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

Support clobber_abi in MSP430 inline assembly #131310

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Oct 5, 2024

This supports clobber_abi which is one of the requirements of stabilization mentioned in #93335.

Refs: Section 3.2 "Register Conventions" in MSP430 Embedded Application Binary Interface

cc @cr1901

r? @Amanieu

@rustbot label +O-msp430 +A-inline-assembly

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-msp430 labels Oct 5, 2024
@cr1901
Copy link
Contributor

cr1901 commented Oct 6, 2024

Hmmm, I is clobber_abi relatively new? I don't remember it, but I'm also a bit out of practice.

@taiki-e Thanks for providing the relevant section of the manual, so I could refresh. It looks good to me, except for two questions.

  • Should clobber_abi also support abi-msp430-interrupt?
    image

  • Why is this not call void asm sideeffect, and only asm sideeffect?

    // CHECK-LABEL: @clobber_abi
    // CHECK: asm sideeffect "", "={r11},={r12},={r13},={r14},={r15}"()
    #[no_mangle]
    pub unsafe fn clobber_abi() {
        asm!("", clobber_abi("C"), options(nostack, nomem, preserves_flags));
    }
    

@taiki-e
Copy link
Member Author

taiki-e commented Oct 6, 2024

@cr1901

Thanks for pointing that out, I missed that because it is unstable. However, it appears that *-interrupt ABIs are not currently supported in clobber_abi on other architectures as well. (Since dependence on other unstable features may prevent stabilization, their lack of support is maybe intentional.)

"x86-interrupt",
"avr-interrupt",
"avr-non-blocking-interrupt",
"riscv-interrupt-m",
"riscv-interrupt-s",

x86/x86_64:

InlineAsmArch::X86 => match name {
"C" | "system" | "efiapi" | "cdecl" | "stdcall" | "fastcall" => {
Ok(InlineAsmClobberAbi::X86)
}
_ => Err(&["C", "system", "efiapi", "cdecl", "stdcall", "fastcall"]),
},
InlineAsmArch::X86_64 => match name {
"C" | "system" if !target.is_like_windows => Ok(InlineAsmClobberAbi::X86_64SysV),
"C" | "system" if target.is_like_windows => Ok(InlineAsmClobberAbi::X86_64Win),
"win64" | "efiapi" => Ok(InlineAsmClobberAbi::X86_64Win),
"sysv64" => Ok(InlineAsmClobberAbi::X86_64SysV),
_ => Err(&["C", "system", "efiapi", "win64", "sysv64"]),
},

riscv:

InlineAsmArch::RiscV32 | InlineAsmArch::RiscV64 => match name {
"C" | "system" | "efiapi" => Ok(InlineAsmClobberAbi::RiscV),
_ => Err(&["C", "system", "efiapi"]),
},

(clobber_abi for AVR is not yet supported)


  • Why is this not call void asm sideeffect, and only asm sideeffect?

This is because it is not a call void asm, but a very long one: call { i16, i16, i16, i16, i16 } asm

That test is based on the same test for s390x added in #130630, and call ... asm for s390x was call { i32, i32, i32, i32, i32, i32, i32, double, double, double, double, double, double, double, double } asm.
In #130630, I had originally intended to write everything call ..., but realized it would be too long for clobber_abi and omitted it. I left it at the other ones (flags and clobber-only registers) because I thought if they were not call void asm, something was wrong.

FYI, for x86_64, it is call { i32, i32, i32, i32, i32, i32, i32, i32, i32, float, float, float, float, float, float, float, float, float, float, float, float, float, float, float, float } asm,
and call ... asm sideeffect "", is omitted in codegen test for clobber_abi:

// CHECK: ={ax},={cx},={dx},={si},={di},={r8},={r9},={r10},={r11},={xmm0},={xmm1},={xmm2},={xmm3},={xmm4},={xmm5},={xmm6},={xmm7},={xmm8},={xmm9},={xmm10},={xmm11},={xmm12},={xmm13},={xmm14},={xmm15},~{xmm16},~{xmm17},~{xmm18},~{xmm19},~{xmm20},~{xmm21},~{xmm22},~{xmm23},~{xmm24},~{xmm25},~{xmm26},~{xmm27},~{xmm28},~{xmm29},~{xmm30},~{xmm31},~{k0},~{k1},~{k2},~{k3},~{k4},~{k5},~{k6},~{k7},~{st},~{st(1)},~{st(2)},~{st(3)},~{st(4)},~{st(5)},~{st(6)},~{st(7)},~{tmm0},~{tmm1},~{tmm2},~{tmm3},~{tmm4},~{tmm5},~{tmm6},~{tmm7},~{dirflag},~{fpsr},~{flags},~{memory}

@cr1901
Copy link
Contributor

cr1901 commented Oct 6, 2024

Since dependence on other unstable features may prevent stabilization, their lack of support is maybe intentional.

That's fair. AFAIK no one is driving the current RFC for unified interrupt calling conventions forward, so perhaps no impetus to support the interrupt calling conventions elsewhere right now.

This is because it is not a call void asm, but a very long one: call { i16, i16, i16, i16, i16 } asm

Ahhh okay I see, CHECK strings need not match at the beginning. I've only ever written "full" CHECK strings.

This PR looks fine to me.

@Amanieu
Copy link
Member

Amanieu commented Oct 11, 2024

Interrupt calling conventions can't be supported by clobber_abi because they clobber reserved registers, which is not supported by back-ends.

@Amanieu
Copy link
Member

Amanieu commented Oct 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 3743618 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Oct 11, 2024
Support clobber_abi in MSP430 inline assembly

This supports `clobber_abi` which is one of the requirements of stabilization mentioned in rust-lang#93335.

Refs: Section 3.2 "Register Conventions" in [MSP430 Embedded Application Binary Interface](https://www.ti.com/lit/an/slaa534a/slaa534a.pdf)

cc `@cr1901`

r? `@Amanieu`

`@rustbot` label +O-msp430
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#124874 (intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics)
 - rust-lang#130962 (Migrate lib's `&Option<T>` into `Option<&T>`)
 - rust-lang#131289 (stabilize duration_consts_float)
 - rust-lang#131310 (Support clobber_abi in MSP430 inline assembly)
 - rust-lang#131546 (Make unused_parens's suggestion considering expr's attributes.)
 - rust-lang#131565 (Remove deprecation note in the `non_local_definitions` lint)
 - rust-lang#131576 (Flatten redundant test module `run_make_support::diff::tests::tests`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#124874 (intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics)
 - rust-lang#130962 (Migrate lib's `&Option<T>` into `Option<&T>`)
 - rust-lang#131289 (stabilize duration_consts_float)
 - rust-lang#131310 (Support clobber_abi in MSP430 inline assembly)
 - rust-lang#131546 (Make unused_parens's suggestion considering expr's attributes.)
 - rust-lang#131565 (Remove deprecation note in the `non_local_definitions` lint)
 - rust-lang#131576 (Flatten redundant test module `run_make_support::diff::tests::tests`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9e72070 into rust-lang:master Oct 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 12, 2024
@taiki-e taiki-e deleted the msp430-clobber-abi branch October 12, 2024 12:07
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2024
Rollup merge of rust-lang#131310 - taiki-e:msp430-clobber-abi, r=Amanieu

Support clobber_abi in MSP430 inline assembly

This supports `clobber_abi` which is one of the requirements of stabilization mentioned in rust-lang#93335.

Refs: Section 3.2 "Register Conventions" in [MSP430 Embedded Application Binary Interface](https://www.ti.com/lit/an/slaa534a/slaa534a.pdf)

cc ``@cr1901``

r? ``@Amanieu``

``@rustbot`` label +O-msp430
@rustbot rustbot added the A-inline-assembly Area: Inline assembly (`asm!(…)`) label Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) O-msp430 S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants