MIR inlining: allow backends to opt-in to inlining intrinsics

[RalfJung]

View all comments

This is particularly useful for type_id_eq which we currently inline by hand.
Cc @scottmcm who suggested this
Cc @saethlin because inlining

I was a bit confused that apparently the inlining pass checks three times whether something is an intrinsic and hence cannot be inlined (resolve_callsite, check_mir_is_available, is_inline_valid_on_fn). I didn't want to duplicate the new logic in 3 places and I didn't know which place would be the right one -- please have a look, I hope what I picked makes sense.

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @oli-obk, @lcnr

[rustbot]

r? @adwinwhite

rustbot has assigned @adwinwhite.
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 73 candidates
• Random selection from 18 candidates

[RalfJung]

The two "rotate" ones here are not a clear-cut choice: their fallback body uses funnel shifts. Cranelift and GCC don't have implementations for those. So if we inline the fallback body of rotate_left (because the standard library is compiled with the LLVM backend), then they'll end up compiling the funnel shift fallbacks instead of the rotation intrinsics.

I'm going to check whether this makes any perf difference in our benchmarks, though I am not sure if they exercise this codepath.

Cc @bjorn3 @antoyo @GuillaumeGomez -- how common is it to mix the LLVM-built standard library with cranelift/GCC for the remaining crates?

View changes since the review

[antoyo]

Cc @bjorn3 @antoyo @GuillaumeGomez -- how common is it to mix the LLVM-built standard library with cranelift/GCC for the remaining crates?

This is the default for rustup distribution.

[RalfJung]

Okay. So we'll have to figure out how to tradeoff optimizing for the common (LLVM) case vs alternative backends.

[RalfJung]

It would have been slightly nicer to do this check in check_mir_is_available instead of what I did here because there we can log a reason for why we're not inlining -- but that's too late, since we have to actually replace the instance here to be able to later fetch the fallback body.

View changes since the review

[RalfJung]

@bors try
@rust-timer queue

[bjorn3]

Instead of inlining the already in the MIR inliner, would it be possible to do the inlining inside the codegen backend right after calling instance_mir using a helper function similar to what optimize_use_clone does? By the way looks like someone forgot to add the optimize_use_clone call to cg_clif.

[RalfJung]

The point of the MIR inliner is to do the work once in generic code rather than many times on monomorphized code, so that doesn't seem so useful to me. Also I don't like these ad-hoc transformations during monomorphization.

My plan was to just remove the rotation intrinsics from the list again, under the assumption that perf will come back neutral.

[RalfJung]

Uh, not sure what's up with that mir-opt test failure -- when I run it locally with --bless it works fine and there's no diff...

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4337910 (4337910a70ebea23eb598a97d7694c6bda48ec2b, parent: 99eed207b47aca1fec5c665531db8e948a92d0ca)

[rust-timer]

Finished benchmarking commit (4337910): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.2%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -2.4%)

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

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

Cycles

Results (secondary -0.3%)

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

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

Binary size

Results (primary 0.6%)

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
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

Bootstrap: 499.454s -> 505.539s (1.22%)
Artifact size: 397.23 MiB -> 397.17 MiB (-0.02%)

[saethlin]

similar to what optimize_use_clone does?

optimize_use_close does a horrendous hack around our lack of post-mono MIR opts which makes the feature impose a large compile-time penalty. That is a hack and should be removed when the feature is stabilized.

[RalfJung]

Doesn't look like the rotate ones are worth it so I'll remove them again for now.

[rustbot]

The Cranelift subtree was changed

cc @bjorn3

The GCC codegen subtree was changed

cc @antoyo, @GuillaumeGomez

[adwinwhite]

r? wg-mir-opt