improve codegen of fmt_num to delete unreachable panic

[iximeow]

it seems LLVM doesn't realize that curr is always decremented at least once in either loop formatting characters of the input string by their appropriate radix, and so the later &buf[curr..] generates a check for out-of-bounds access and panic. this is unreachable in reality as even for x == T::zero() we'll produce at least the character Self::digit(T::zero()), yielding at least one character output, and curr will always be at least one below buf.len().

adjust fmt_int to make this fact more obvious to the compiler, which fortunately (or unfortunately) results in a measurable performance improvement for workloads heavy on formatting integers.

in the program i'd noticed this in, you can see the cmp $0x80,%rdi; ja 7c here, which branches to a slice index fail helper:

where after this change the function is broadly similar, but smaller, with one fewer registers updated in each pass through the loop in addition the never-taken cmp/ja being gone:

this represents a ~2-3% difference in runtime in my admittedly comically i32-formatting-bound use case (printing x86 instructions, including i32 displacements and immediates) as measured on a ryzen 9 3950x.

the impact on <impl LowerHex for i8>::fmt is both more dramatic and less impactful: it continues to have a loop that is evaluated at most twice, though the compiler doesn't know that to unroll it. the generated code there is identical to the impl for i32. there, the smaller loop body has less effect on runtime, and removing the never-taken slice bounds check is offset by whatever address recalculation is happening with the lea/add/neg at the end of the loop. it behaves about the same before and after.

---

i initially measured slightly better outcomes using unreachable_unchecked() here instead, but that was hacking on std and rebuilding with -Z build-std on an older rustc (nightly 5b377cece, 2023-06-30). it does not yield better outcomes now, so i see no reason to proceed with that approach at all.

initial notes about that, seemingly irrelevant on modern rustc

i went through a few tries at getting llvm to understand the bounds check isn't necessary, but i should mention the _best_ i'd seen here was actually from the existing `fmt_int` with a diff like ```diff if x == zero { // No more digits left to accumulate. break; }; } } + + if curr >= buf.len() { + unsafe { core::hint::unreachable_unchecked(); } + } let buf = &buf[curr..]; ```

posting a random PR to rust-lang/rust to do that without a really really compelling reason seemed a bit absurd, so i tried to work that into something that seems more palatable at a glance. but if you're interested, that certainly produced better (x86_64) code through LLVM. in that case with buf.iter_mut().rev() as the iterator, <impl LowerHex for i8>::fmt actually unrolls into something like

put_char(x & 0xf);
let mut len = 1;
if x > 0xf {
  put_char((x >> 4) & 0xf);
  len = 2;
}
pad_integral(buf[buf.len() - len..]);

it's pretty cool! <impl LowerHex for i32>::fmt also was slightly better. that all resulted in closer to an 6% difference in my use case.

---

i have not looked at formatters other than LowerHex/UpperHex with this change, though i'd be a bit shocked if any were worse.

(i have absolutely no idea how you'd regression test this, but that might be just my not knowing what the right tool for that would be in rust-lang/rust. i'm of half a mind that this is small and fiddly enough to not be worth landing lest it quietly regress in the future anyway. but i didn't want to discard the idea without at least offering it upstream here)

[rustbot]

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[clubby789]

@bors try @rust-timer queue

[bors]

⌛ Trying commit e7fbf72 with merge baf578c...

[bors]

☀️ Try build successful - checks-actions
Build commit: baf578c (baf578c47b9f7cd1d2cb6f622990e99efc2f43cd)

[workingjubilee]

(i have absolutely no idea how you'd regression test this, but that might be just my not knowing what the right tool for that would be in rust-lang/rust. i'm of half a mind that this is small and fiddly enough to not be worth landing lest it quietly regress in the future anyway. but i didn't want to discard the idea without at least offering it upstream here)

@iximeow this can in fact be regression-tested! we'd use a codegen (llvm level) or assembly (x86-64 level) test. they use FileCheck to verify that a certain pattern is fulfilled, and can be parameterized with a bit of fussing with their DSL and regex so that e.g. it doesn't matter exactly which r[axsbi0-9]{2} register you're using. a codegen test is slightly preferred for this as it would be "platform-independent" to some degree, but i'd be happy to accept this with an assembly test.

[rust-timer]

Finished benchmarking commit (baf578c): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.5%	[1.4%, 1.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	4
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.4%]	32
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	4

Bootstrap: 669.372s -> 670.355s (0.15%)
Artifact size: 312.78 MiB -> 312.85 MiB (0.02%)

[iximeow]

are there codegen tests for how rustc compiles core? (or std?) those are what i couldn't find under library/test. there are benches under library/core/benches/fmt.rs that seem relevant but i couldn't follow those to test executions so i'm not sure if they're intended for more one-off spot checking of changes.

[iximeow]

Overall result: ❌ regressions - no action needed

🤨 or given that i expected this at worst to have no effect, maybe i should double-check how core/benches/fmt.rs looks before/after too..

[workingjubilee]

@iximeow That performance suite, and the remark, is about compiler tests, primarily. There's a runtime perf suite, but it showed no change. It's not surprising to me that rustc's happy path instruction counts aren't greatly affected by this change (this branch, after all, should not be hit), and the two tests it is affected on are fairly "noisy" tests and also reflect synthetic stress tests.

are there codegen tests for how rustc compiles core? (or std?) those are what i couldn't find under library/test.

It will be under tests/codegen not library/tests, sadly. The library/tests are only for those that can be run by the "usual" Rust test suite.

An example that is relevant is this, with its CHECK-NOT: panic:

https://github.com/rust-lang/rust/blob/e3df96cfda905301fc8514e000f942222c1ab6ad/tests/codegen/vec-shrink-panik.rs

[workingjubilee]

@iximeow the perf bot's default remarks are also based on instructions executed. note that the wall time is reduced:

this, combined with the (slight) binary size reduction, in generated artifacts (admittedly, the only non-synthetic mark here is helloworld):

suggests to me that this may in fact be resulting in smaller code sizes with better cache layout. also, I'd be happy to accept the unreachable_unchecked if given an LLVMIR codegen test specifically.

[iximeow]

i added fmt_int_no_panic.rs in cea973f and quickly figured out i don't know how to get it to test that the right function does not panic. the fact that the test passes is misleading.

for tests like vecdeque_no_panic.rs it seems that the assertions are strongly reliant on the tested functions being inlined, and in cases like those if front or back were outlined the test might appear to pass because no panic would exist in fn dont_panic, but a panic may exist in fn front when called. so it looks like that test would want to include a // CHECK-NOT: call to be certain we're checking all the IR. same goes for vec-shrink-panik etc.

this is a problem for the new fmt_into_no_panic because core::fmt::write is resistant to inlining and the function i want to check is a few calls away from what i can write in the test. fn format_int_doesnt_panic produces IR like...

; Function Attrs: nonlazybind uwtable
define noundef zeroext i1 @format_int_doesnt_panic(ptr noalias noundef align 8 dereferenceable(24) %s) unnamed_addr #0 {
_ZN4core3fmt9Arguments12as_const_str17hf62647c4668d295dE.exit.i:
  %_14 = alloca [1 x %"core::fmt::rt::Argument<'_>"], align 8
  %args2 = alloca %"core::fmt::Arguments<'_>", align 8
  %_7 = alloca [1 x %"core::fmt::rt::Argument<'_>"], align 8
  %args = alloca %"core::fmt::Arguments<'_>", align 8
  call void @llvm.lifetime.start.p0(i64 48, ptr nonnull %args)
  call void @llvm.lifetime.start.p0(i64 16, ptr nonnull %_7)
  store ptr @alloc_914b2c69d7eca30497b9feaf15ac92f1, ptr %_7, align 8
  %0 = getelementptr inbounds i8, ptr %_7, i64 8
  store ptr @"_ZN4core3fmt3num52_$LT$impl$u20$core..fmt..LowerHex$u20$for$u20$u8$GT$3fmt17hac9971cd5590704cE", ptr %0, align 8
  store ptr @alloc_b99730e73100e73a81f4fbfe74b3821d, ptr %args, align 8
  %1 = getelementptr inbounds i8, ptr %args, i64 8
  store i64 1, ptr %1, align 8
  %2 = getelementptr inbounds i8, ptr %args, i64 32
  store ptr null, ptr %2, align 8
  %3 = getelementptr inbounds i8, ptr %args, i64 16
  store ptr %_7, ptr %3, align 8
  %4 = getelementptr inbounds i8, ptr %args, i64 24
  store i64 1, ptr %4, align 8
; call core::fmt::write
 [snip, past here doesn't matter much]

where the assertion i want to write is really that _ZN4core3fmt3num52_$LT$impl$u20$core..fmt..LowerHex$u20$for$u20$u8$GT$3fmt17hac9971cd5590704cE doesn't panic.

but in this .ll all we get is the function declaration,

; core::fmt::num::<impl core::fmt::LowerHex for u8>::fmt
; Function Attrs: nonlazybind uwtable
declare noundef zeroext i1 @"_ZN4core3fmt3num52_$LT$impl$u20$core..fmt..LowerHex$u20$for$u20$u8$GT$3fmt17hac9971cd5590704cE"(ptr noalias noundef readonly align 1 dereferenceable(1), ptr noalias noundef align 8 dereferenceable(64)) unnamed_addr #0

i was hoping that maybe by making the crate type sufficiently statically linked that i could get a .ll with this function filled in, but i don't see how. at least then writing a correct test would be reduced to "write a CHECK-LABEL for a function that does not appear in the test program"...

i couldn't see similar prior art in other forms of CHECK{,-NOT}: panic under tests/codegen. it looks like all cases rely on the function to be checked to be inlined into the top-level function being tested. @workingjubilee am i missing another detail of the test suite or is this particularly tricky to test?

(i also added benches for formatting u8::MAX and 0u8 similar to the larger types. it seems on a current rustc there's not much benefit to unreachable_unchecked() vs changing the loop as i've done in this patch. so i've <details>'d that section of the PR description)

[workingjubilee]

@iximeow Oh, no, you're right, this is tricky to test. Hrm.

[workingjubilee]

@saethlin do we have a sufficient combination of commands such that with enough

• "we have LTO at home"
• the LTO: -Zcross-crate-inline-threshold=yes

we can make this codegen-testable anyway?

[saethlin]

it looks like all cases rely on the function to be checked to be inlined into the top-level function being tested

That is true and I really do not like that approach. Playing games with the inliner rules tends to have undesired effects; for example it's possible if Formatter::pad_integral got inlined into this function that LLVM might manage to do the analysis you're doing here by hand and eliminate the check without the implementation improvement in this PR.

In addition, codegen tests are built against whatever sysroot happens to be specified by the settings in config.toml. It might have debug assertions or it might not. It might even have optimizations disabled.

I recently added a test based on using -Zbuild-std then analyzing the build artifacts: https://github.com/rust-lang/rust/blob/020bbe46bdf613c0b8211984eaa8e5fe97d6c92d/tests/run-make/compiler-builtins/rmake.rs. Just like it's important that we not only run the test suite that we run the test suite on the build artifacts going out the door for codegen tests like this that are trying to check on the implementation of the standard library, what I'd prefer to see is a test that analyzes the standard library code under normal user conditions. I'm not sure implementing this is a reasonable ask for this PR.

---

The usual LTO at home flags are:

-Copt-level=3 -Zcross-crate-inline-threshold=yes -Zmir-opt-level=3 -Zinline-mir-hint-threshold=10000 -Zinline-mir-threshold=10000

Full LLVM optimizations, the LTO at home flag, enable MIR inlining in all possible cases then crank up the thresholds to an absurd level. (be aware that just applying such a high MIR inlining threshold to arbitrary crates will cause absurd memory usage, even my beefy desktop cannot compile syn with these flags)

But this doesn't help in part because of what I said above: The sysroot has already been compiled with a different set of flags that make LTO at home fail to apply. (as an optimization to compile time grumble grumble) #115306 we only encode MIR which is deduced to be useful to other crates when compiling the current crate. The compilation of core has decided that core::fmt::write will not be useful for other crates, and so it becomes ineligible for inlining. Except by LLVM's LTO, because the standard library build we distribute is special-cased: We prune rustc's IR but always emit LLVM's IR for all functions via -Cembed-bitcode=yes.

[the8472]

An assembly test with -Clto might work.

[the8472]

We discussed this in a T-libs meeting. Given the difficulty of writing a codegen test that properly inlines everything we're ok with merging this without a test.

AIUI the test currently isn't working as intended? If so it should be removed from the PR and someone wants to add a working codegen test that can be done in a followup PR.

[iximeow]

AIUI the test currently isn't working as intended?

correct, i'd included it in the hopes it was close to being useful (alas!) - i've removed it but kept the benches for u8 formatting as those are informative.

[workingjubilee]

Ah, thank you! Sorry about the everything,

@bors r+ rollup=never

[bors]

📌 Commit a54d6ad has been approved by workingjubilee

It is now in the queue for this repository.

[bors]

⌛ Testing commit a54d6ad with merge 22bcb81...

[bors]

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 22bcb81 to master...

[rust-timer]

Finished benchmarking commit (22bcb81): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.3%, -2.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.3%, -2.1%]	3

Cycles

Results (primary -3.3%, secondary 1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-3.3%	[-3.3%, -3.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.3%	[-3.3%, -3.3%]	1

Binary size

Results (primary 0.0%, secondary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	4

Bootstrap: 786.257s -> 787.179s (0.12%)
Artifact size: 335.35 MiB -> 335.33 MiB (-0.00%)