Add should_skip_bounds_check to MIR Builder

[Bryntet]

This PR attempts to improve compiletimes by improving MIR generation, with the hope of making it so the chosen codegen backend has to do less work (this also comes with the benefit of eliding some bounds checks in debug builds!)

take the following two playground examples, and see how they always emit a bounds check in MIR

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=a10561edd6a4a37a7326e785723aa95d

eliding in this case is sound because the literal is less than the length of the array (it's equivalent to doing the runtime bounds check, but at comp-time )

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=a791356ddb6293f01138632d6a7671af

in this case it's a bit less obvious why it's sound to always elide the bounds check, but since u8::MAX < 256 any u8 value can have its bounds elided

in the future, it might be nice to add support for pattern types here?

I have tested with a subset of rustc-perf tests locally and instruction count wise it seems to be an improvement, but probably a good idea to run a perf test anyways

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 16 candidates

[Bryntet]

One question I have is: is it possible to get a circular recursion of ExprKind::Scope? this would ICE due to hitting recursion limit in my PR.

[JonathanBrouwer]

@bors try @rust-timer queue

[nnethercote]

I'm not a MIR expert, I'll try someone I think knows more:

r? @saethlin

[JonathanBrouwer]

with the hope of making it so the linker has to do less work

It would be LLVM that does less work, not the linker right?

[Bryntet]

It would be LLVM that does less work, not the linker right?

LLVM is a linker right? I tried to keep the language vague since cranelift and alternatives also exist, and improvement to MIR should be an improvement for all these backends. Maybe backend is the word I was looking for?

[JonathanBrouwer]

It would be LLVM that does less work, not the linker right?

LLVM is a linker right? I tried to keep the language vague since cranelift and alternatives also exist, and improvement to MIR should be an improvement for all these backends. Maybe backend is the word I was looking for?

LLVM is a codegen backend just like cranelift, gcc, etc.
Codegen backends take crates or parts of crates and compile them to an .o ELF file.
Linkers (like lld, mold, wild) take all the seperately produced ELF files and link them together (and do some other magic)

[Bryntet]

I can't say I'm intimately familiar with how LLVM works, so it would be good if someone could say that this changing in this way is ok?

[Bryntet]

I assume this is fine though, because we don't read from these two pointers here, so it just being poisoned should be ok

[Bryntet]

and this changing would be due to it being in debug, where LLVM doesn't elide bounds checks

[rust-bors]

☀️ Try build successful (CI)
Build commit: 78a8d36 (78a8d36332a4b4054f62c90406b543c67784ea80, parent: 98e7077b903559d7a4fafb775cd5292cc9427b67)

[rust-timer]

Finished benchmarking commit (78a8d36): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary -1.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.2%, -2.7%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.5%	[-3.2%, 2.8%]	4

Cycles

Results (primary 1.1%, secondary -2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

Results (primary -0.1%, secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	12
Improvements ✅ (secondary)	-0.4%	[-0.4%, -0.4%]	1
All ❌✅ (primary)	-0.1%	[-0.4%, 0.6%]	13

Bootstrap: 479.865s -> 478.813s (-0.22%)
Artifact size: 395.05 MiB -> 396.89 MiB (0.46%)

[kpreid]

Some review comments as a person who occasionally reads rustc code and would like it to be less cryptic on average. (Also one bug, I think.)

View changes since this review

[kpreid]

I think it would be good to give this function a name that documents what it does, such as expr_always_less_than_len(). “kind” doesn’t say much because, the kind of what?

[kpreid]

How about calling this something like index_expr_id?

[kpreid]

I don’t think this is correct. Assuming I am correct that this corresponds to a Rust expression like array[index as usize], such a cast can produce large usize, because the cast is sign-extending:

fn main() {
    let arr = [0; 256];
    arr[(-1i16) as usize];
}

error: this operation will panic at runtime
 --> src/main.rs:3:5
  |
3 |     arr[(-1i16) as usize];
  |     ^^^^^^^^^^^^^^^^^^^^^ index out of bounds: the length is 256 but the index is usize::MAX

[kpreid]

It would be nice if the same optimization was available for infallible conversions through array[usize::from(index)], which are otherwise superior when starting with u8 or u16. I imagine that is much harder to implement, though.

[kpreid]

This could be combined into a single pattern instead of using a guard.

Suggested change

	ExprKind::Literal { lit, .. } if let LitKind::Int(num, _) = lit.node => {
	ExprKind::Literal { lit: Lit { node: LitKind::Int(num, _), .. }, .. } => {

[kpreid]

You have a case for literal values, and a case for casts, but the case for casts doesn’t take advantage of the fact that the cast might be of a literal, like character_table[b'x' as usize], which seems like an easy opportunity to me. (I also notice that this code won’t look at named constants at all, but I assume resolving them to their values is nontrivial here.)