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.

- stable
- "1.81" # host MSRV
- nightly-2025-08-05 # some tests use unstable features, but avoid LLVM21 due to https://github.com/rust-lang/rust/issues/146065
- nightly-2025-09-25
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to revert this back to just nightly, so we catch any future nightly issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice, I just assumed it was pinned before the regression. Changed.

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