Support `clobber_abi` in AVR inline assembly #131323

jfrimmel · 2024-10-06T11:14:10Z

This PR implements the clobber_abi part necessary to eventually stabilize the inline assembly for AVR. This is tracked in #93335.
This is heavily inspired by the sibling-PR #131310 for the MSP430. I've explained my reasoning in the first commit message in detail, which is reproduced below for easier reviewing:

This follows the ABI documentation of AVR-GCC:

The [...] call-clobbered general purpose registers (GPRs) are registers that might be destroyed (clobbered) by a function call.

R18–R27, R30, R31

These GPRs are call clobbered. An ordinary function may use them without restoring the contents. [...]

R0, T-Flag

The temporary register and the T-flag in SREG are also call-clobbered, but this knowledge is not exposed explicitly to the compiler (R0 is a fixed register).

Therefore this commit lists the aforementioned registers r18–r27, r30 and r31 as clobbered registers. Since the r0 register (listed above as well) is not available in inline assembly at all (potentially because the AVR-GCC considers it a fixed register causing the register to never be used in register allocation and LLVM adopting this), there is no need to list it in the clobber list (the r0-variant is not even available). A comment was added to ensure, that the r0 gets added to the clobber-list once the register gets usable in inline ASM.
Since the SREG is normally considered clobbered anyways (unless the user supplies the preserve_flags-option), there is no need to explicitly list a bit in this register (which is not possible to list anyways).

Note, that this commit completely ignores the case of interrupts (that are described in the ABI-specification), since every register touched in an ISR need to be saved anyways.

r? @Amanieu

@rustbot label +O-AVR

Amanieu · 2024-10-11T12:51:47Z

compiler/rustc_target/src/asm/avr.rs

@@ -106,6 +106,7 @@ def_regs! {
            "the stack pointer cannot be used as an operand for inline asm",
        #error = ["r0", "r1", "r1r0"] =>
            "r0 and r1 are not available due to an issue in LLVM",
+            // if this issue is resolved, then R0 has to be added to clobbered registers


We need to carefully keep track of LLVM here: if LLVM ceases to treat these registers as reserved then we must immediately add them to the set of clobbered registers, otherwise incorrect code will be generated. This comment should be updated to reflect this.

Also, while reviewing the GCC docs, it seems that R1 always holds a value of 0. If this is a guarantee that asm can use then it should be documented, similar to how on x86 we guarantee the direction flag is always clear when entering or exiting inline asm.

Thank you for the feedback!

Do you have an idea on how to track LLVM here? I'd expect the ABI to be stable, but I don't know on how to test this, except for producing register-heavy code and hoping, that the "no more fixed register" would be used causing the test to fail. This seems rather fragile, though.
Or do you "just" want to phrase the comment different, so that the required immediate action is more clear?

For the second issue, I'll go ahead and document, that the R1 register needs to be restored if altered.

It's enough to just reword the comment.

I've reworded the comment, please have a look if this works for you.

Regarding the R1 register content: the unstable-book already documents, that the value needs to be restored if altered:

rust/src/doc/unstable-book/src/language-features/asm-experimental-arch.md

Line 156 in 17a19e6

| AVR | `r0`, `r1`, `r1r0` | Due to an issue in LLVM, the `r0` and `r1` registers cannot be used as inputs or outputs. If modified, they must be restored to their original values before the end of the block. |

jfrimmel · 2024-10-14T16:46:32Z

@rustbot labels: +A-inline-assembly
@rustbot ready

taiki-e · 2024-11-02T07:13:22Z

compiler/rustc_target/src/asm/avr.rs

        #error = ["r0", "r1", "r1r0"] =>
            "r0 and r1 are not available due to an issue in LLVM",


(IIUC since r1 is a zero register, r1 and r1r0 cannot be used as an operand for inline asm, regardless of an LLVM issue.)

I think, the fact that r1 is the zero-reg is the cause for the registers not being available for inline asm (although I'm not sure). For now, I think this is fine (please correct me if you think otherwise!).

For now, I think this is fine (please correct me if you think otherwise!).

Agreed. We may need to figure it out in some way before stabilization, but at least it should not be a blocker of clobber_abi implementation.

It would actually be nice to make the error message here clearer than "due to an issue in LLVM". The reason is that r0 is reserved as a scratch register and r1 is reserved as a constant zero register by LLVM.

tests/codegen/asm-clobber_abi-avr.rs

tests/codegen/asm/avr-clobbes.rs

taiki-e · 2024-11-02T08:42:25Z

Btw, AVRTiny¹ appears to have a different convention (r0-r15 are unavailable, r16-r17 are similar to r0-r1 in AVR, r18-r19 are call-saved).

avrtiny, attiny4, attiny5, attiny9, attiny10, attiny20, attiny40, attiny102, and attiny104 are marked as FamilyTiny in LLVM ↩

jfrimmel · 2024-11-02T23:41:50Z

Hm, I think in order to properly address the reduced tiny cores, one would need to wait for #131651 and parse the -Ctarget-cpu-flag to detect such a core. I suppose, this change would also be necessary for the general inline assembly and thus is somewhat out of scope of this PR. I see two options:

merge this as-is and perform the necessary changes in a follow-up PR
close this and wait for Create a generic AVR target: avr-unknown-unknown #131651 to be merged, then fix the reduced tiny cores and then the clobber ABI.

taiki-e · 2024-11-03T05:08:42Z

I suppose, this change would also be necessary for the general inline assembly and thus is somewhat out of scope of this PR.

I think it is indeed true that inline assembly support for AVRTiny is already broken (registers that should be reserved are not reserved) and therefore does not need to be fixed in this PR.
(EDIT: It should need to be fixed before stabilization, so I added it to #93335 (comment) 's list.)

parse the -Ctarget-cpu-flag to detect such a core.

I think it is better to refer to the tinyencoding target feature as LLVM does, instead of parsing the target-cpu.
(I think this could be implemented in a similar way to #132472 (comment).)

Amanieu · 2024-11-15T15:31:41Z

Could you please fix the wording as suggested in #131323 (comment), then this can be merged.

Amanieu · 2024-11-28T01:15:29Z

@rustbot author

Those are reserved as per the GCC (and thus LLVM) ABI, which is distinct from an issue. The rewording was requested in this [review]. [review]: rust-lang#131323 (comment)

jfrimmel · 2024-11-28T12:00:17Z

Sorry, I've missed your review feedback. I've reworded the line in question.

@rustbot ready

Amanieu · 2024-11-28T13:19:46Z

@bors r+

bors · 2024-11-28T13:19:49Z

📌 Commit 297f2a5 has been approved by Amanieu

It is now in the queue for this repository.

bors · 2024-11-28T14:04:54Z

☔ The latest upstream changes (presumably #133568) made this pull request unmergeable. Please resolve the merge conflicts.

This commit adds the relevant registers to the list of clobbered regis- ters (part of rust-lang#93335). This follows the [ABI documentation] of AVR-GCC: > The [...] call-clobbered general purpose registers (GPRs) are > registers that might be destroyed (clobbered) by a function call. > > - **R18–R27, R30, R31** > > These GPRs are call clobbered. An ordinary function may use them > without restoring the contents. [...] > > - **R0, T-Flag** > > The temporary register and the T-flag in SREG are also call- > clobbered, but this knowledge is not exposed explicitly to the > compiler (R0 is a fixed register). Therefore this commit lists the aforementioned registers `r18–r27`, `r30` and `r31` as clobbered registers. Since the `r0` register (listed above as well) is not available in inline assembly at all (potentially because the AVR-GCC considers it a fixed register causing the register to never be used in register allocation and LLVM adopting this), there is no need to list it in the clobber list (the `r0`-variant is not even available). A comment was added to ensure, that the `r0` gets added to the clobber-list once the register gets usable in inline ASM. Since the SREG is normally considered clobbered anyways (unless the user supplies the `preserve_flags`-option), there is no need to explicitly list a bit in this register (which is not possible to list anyways). Note, that this commit completely ignores the case of interrupts (that are described in the ABI-specification), since every register touched in an ISR need to be saved anyways. [ABI documentation]: https://gcc.gnu.org/wiki/avr-gcc#Call-Used_Registers

Those are reserved as per the GCC (and thus LLVM) ABI, which is distinct from an issue. The rewording was requested in this [review]. [review]: rust-lang#131323 (comment)

jfrimmel · 2024-11-28T15:13:08Z

rebased

@rustbot ready

Amanieu · 2024-11-28T15:19:47Z

@bors r+

bors · 2024-11-28T15:19:49Z

📌 Commit 67d2f3f has been approved by Amanieu

It is now in the queue for this repository.

rustbot assigned Amanieu Oct 6, 2024

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-AVR Target: AVR processors (ATtiny, ATmega, etc.) labels Oct 6, 2024

jfrimmel force-pushed the avr-inline-asm-clobber-abi branch from e6e6b84 to 5d58879 Compare October 6, 2024 12:45

taiki-e mentioned this pull request Oct 6, 2024

Tracking Issue for asm_experimental_arch #93335

Open

Amanieu reviewed Oct 11, 2024

View reviewed changes

jfrimmel force-pushed the avr-inline-asm-clobber-abi branch from 5d58879 to 461c388 Compare October 14, 2024 16:45

rustbot added the A-inline-assembly Area: Inline assembly (`asm!(…)`) label Oct 14, 2024

taiki-e reviewed Nov 2, 2024

View reviewed changes

tests/codegen/asm/avr-clobbes.rs Outdated Show resolved Hide resolved

jfrimmel force-pushed the avr-inline-asm-clobber-abi branch from ccc421a to 558b1ae Compare November 2, 2024 23:37

taiki-e approved these changes Nov 3, 2024

View reviewed changes

Patryk27 approved these changes Nov 15, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2024

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 Nov 28, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 28, 2024

jfrimmel added 4 commits November 28, 2024 16:12

Add test case for the clobber options

d7e0a3e

Move & rename test case to match naming of rust-lang#132456

2bd3bbb

Reword error message of reserved AVR registers

67d2f3f

Those are reserved as per the GCC (and thus LLVM) ABI, which is distinct from an issue. The rewording was requested in this [review]. [review]: rust-lang#131323 (comment)

jfrimmel force-pushed the avr-inline-asm-clobber-abi branch from 297f2a5 to 67d2f3f Compare November 28, 2024 15:12

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2024

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 Nov 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `clobber_abi` in AVR inline assembly #131323

Support `clobber_abi` in AVR inline assembly #131323

jfrimmel commented Oct 6, 2024 •

edited

Loading

Amanieu Oct 11, 2024

jfrimmel Oct 14, 2024

Amanieu Oct 14, 2024

jfrimmel Oct 14, 2024

jfrimmel commented Oct 14, 2024

taiki-e Nov 2, 2024

jfrimmel Nov 2, 2024

taiki-e Nov 2, 2024

Amanieu Nov 6, 2024

taiki-e commented Nov 2, 2024 •

edited

Loading

jfrimmel commented Nov 2, 2024

taiki-e commented Nov 3, 2024 •

edited

Loading

Amanieu commented Nov 15, 2024

Amanieu commented Nov 28, 2024

jfrimmel commented Nov 28, 2024

Amanieu commented Nov 28, 2024

bors commented Nov 28, 2024

bors commented Nov 28, 2024

jfrimmel commented Nov 28, 2024

Amanieu commented Nov 28, 2024

bors commented Nov 28, 2024

		#error = ["r0", "r1", "r1r0"] =>
		"r0 and r1 are not available due to an issue in LLVM",

Support clobber_abi in AVR inline assembly #131323

Are you sure you want to change the base?

Support clobber_abi in AVR inline assembly #131323

Conversation

jfrimmel commented Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfrimmel commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taiki-e commented Nov 2, 2024 • edited Loading

Footnotes

jfrimmel commented Nov 2, 2024

taiki-e commented Nov 3, 2024 • edited Loading

Amanieu commented Nov 15, 2024

Amanieu commented Nov 28, 2024

jfrimmel commented Nov 28, 2024

Amanieu commented Nov 28, 2024

bors commented Nov 28, 2024

bors commented Nov 28, 2024

jfrimmel commented Nov 28, 2024

Amanieu commented Nov 28, 2024

bors commented Nov 28, 2024

Support `clobber_abi` in AVR inline assembly #131323

Support `clobber_abi` in AVR inline assembly #131323

jfrimmel commented Oct 6, 2024 •

edited

Loading

taiki-e commented Nov 2, 2024 •

edited

Loading

taiki-e commented Nov 3, 2024 •

edited

Loading