Skip to content

Conversation

@Dirbaio
Copy link
Contributor

@Dirbaio Dirbaio commented Oct 28, 2024

Requires #878 because asm!(... sym ..) breaks if the symbol name has quote characters. not anymore yay

Defmt string indices are 16 bits, which can be loaded with a single movw.
However, the compiler doesn't know that, so it generates movw+movt or ldr rX, [pc, #offs]
because it sees we're loading an address of a symbol, which could be any 32bit value.
This wastes space, so we load the value with asm manually to avoid this.

#[inline(never)]
unsafe fn testfoobar() {
    info!("foo");
}

before:
00027180 <_ZN11application10testfoobar17h819f2778e910636eE>:
   27180: b580         	push	{r7, lr}
   27182: 466f         	mov	r7, sp
   27184: f000 f954    	bl	0x27430 <_defmt_acquire> @ imm = #0x2a8
   27188: 4803         	ldr	r0, [pc, #0xc]          @ 0x27198 <_ZN11application10testfoobar17h819f2778e910636eE+0x18>
   2718a: f000 fa35    	bl	0x275f8 <_ZN5defmt6export6header17hd841c3329e459a6fE> @ imm = #0x46a
   2718e: e8bd 4080    	pop.w	{r7, lr}
   27192: f000 b95b    	b.w	0x2744c <_defmt_release> @ imm = #0x2b6
   27196: bf00         	nop
   27198: 03 00 00 00  	.word	0x00000003

after:
00027180 <_ZN11application10testfoobar17hc6c595839020df66E>:
   27180: b580         	push	{r7, lr}
   27182: 466f         	mov	r7, sp
   27184: f000 f94e    	bl	0x27424 <_defmt_acquire> @ imm = #0x29c
   27188: f240 0003    	movw	r0, #0x3
   2718c: f000 fa34    	bl	0x275f8 <_ZN5defmt6export6header17h79a831a74481cee1E> @ imm = #0x468
   27190: e8bd 4080    	pop.w	{r7, lr}
   27194: f000 b954    	b.w	0x27440 <_defmt_release> @ imm = #0x2a8

@jonathanpallant
Copy link
Contributor

Thanks for this PR - it looks neat. Once we've resolve the question about hex-encoding symbols, I would support merging this.

@Urhengulas Urhengulas added the breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version label Nov 6, 2024
@jonathanpallant
Copy link
Contributor

(This isn't a breaking change, we just want cargo server-checks to stop complaining about a PR that was committed some time ago)

@Urhengulas Urhengulas requested review from jonathanpallant and removed request for jonathanpallant January 7, 2025 18:54
@Urhengulas Urhengulas added pr waits on: author Pull Request requires changes from the author and removed breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version labels Jan 7, 2025
@jonathanpallant jonathanpallant added this to the Wire Format Version 5 milestone Mar 5, 2025
@Dirbaio
Copy link
Contributor Author

Dirbaio commented Sep 19, 2025

great news: llvm/llvm-project#159420 fixes using quotes in asm!(sym). so #878 is not needed anymore.

I assume it'll land in Rust 1.91. What do we do?

  • Bump MSRV to Rust 1.91+?
  • Check rust version in build.rs and do the optimization only in Rust 1.91+?

@jonathanpallant
Copy link
Contributor

MSRV moving is hard when we have customers on long term support toolchains. A build.rs version switch, or a feature flag, would be fine.

I wonder if this is related to the quote escaping issue in LLVM 21.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Sep 19, 2025

A build.rs version switch, or a feature flag, would be fine.

Implemented a version switch. I wouldn't do a feature flag, it's not very discoverable and it's nice you get the optimization without having to opt-in.

We'll have to wait until the llvm fix lands in nightly/beta to merge this.

I wonder if this is related to the quote escaping issue in LLVM 21.

yep! basically:

@jonathanpallant
Copy link
Contributor

This looks good to me and I'm inclined to merge, but I'd like to do some code size and compile time comparisons first.

// This wastes space, so we load the value with asm manually to avoid this.
let val_arm_optimized = quote!(
let res: u16;
unsafe { ::core::arch::asm!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this assembly code doesn't work on Arm architectures prior to ARMv6T2 (i.e. ARMv4T, ARMv5TE and ARMv6). See Godbolt.

You might need to use https://docs.rs/arm-targets to work out which Arm core is being targeted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could say "Yeah, but those targets have no users", merge it anyway and worry about fixing it if someone actually complains.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no. It doesn't work on thumbv6m-none-eabi either....

https://rust.godbolt.org/z/c9jb9TK36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr waits on: author Pull Request requires changes from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants