Inline mem::size_of & mem::align_of

[tmiasko]

Opened for perf results.

cc @bjorn3
r? @ghost

[bjorn3]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

[bors]

⌛ Trying commit b1c44d3450aeef671b36faa1cdb9f5ebd78cb74b with merge d0efbf33ce34c88893bff82178722695e0031b9c...

[bors]

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

[rust-timer]

Queued d0efbf33ce34c88893bff82178722695e0031b9c with parent fde6927, future comparison URL.

@rustbot label: +S-waiting-on-perf

[rust-timer]

Finished benchmarking try commit (d0efbf33ce34c88893bff82178722695e0031b9c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

[tmiasko]

There is inconsistency in safety of size_of_val_raw function and intrinsic used to implement it.

[bugadani]

Just out of curiousity, looking at webrenderer results, how can this PR change the number of executions for certain queries?

[tmiasko]

The recurring reduction by 4 corresponds to the situation where we avoided creating any monomorphiziations of given item: size_align would be one because it is removed, but I would expect that is also have happened for some of wrapper functions in mem module.

The resolve_instance reduction by 721 corresponds to the reduction in the number of calls terminators inside codegen items. When lowering pass was first enabled (it lowers size_of intrinsic call into a simple MIR statement) this number dropped by 328. This PR removes one layer of indirection when calling both size & align, which is roughly twice that.

[tmiasko]

This approach avoids the perf regression encountered before. I would be interested in potentially landing this, although I am not sure what are general opinions about using intrinsics directly like that.

[the8472]

This could be updated to use usize::BITS

[oli-obk]

I don't feel comfortable with this extensive direct use of intrinsics. If we could turn intrinsics into regular function pointers, then we could do something like we did for transmute and re-export the intrinsic in mem. Since they have a different ABI, doing that without compatibility between rust and rust-intrinsic ABI would be a breaking change.

Maybe we could take a different approach and do it like

rust/library/core/src/ptr/mod.rs

Lines 177 to 185 in ab5b9ae

	#[lang = "drop_in_place"]
	#[allow(unconditional_recursion)]
	pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
	// Code here does not matter - this is replaced by the
	// real drop glue by the compiler.
	// SAFETY: see comment above
	unsafe { drop_in_place(to_drop) }
	}

and not have intrinsics for these at all (so they don't end up with a rust-intrinsic ABI), allowing us to take pointers to them, while still essentially treating them as intrinsics.

Note: I don't know if your approach, the ABI compat approach or the lang item approach are something we want, so maybe let's open a zulip discussion showing that there are perf improvements to be had by doing something here. Then we can MCP whatever we find consensus on, but I think we need more input by the compiler team here, as this kind of change for perf may apply to more than just size_of and align_of

[bjorn3]

drop_in_place is codegened as a real function with real MIR. This real MIR just depends on the generic param unlike regular functions. Intrinsics are codegened inline at the caller site.

[oli-obk]

Hmm right, that is a bit different. So that scheme is too different I guess. While we could make mir building automatically generate the appropriate MIR statements at all call sites to specific lang items, that is something completely new that we haven't done so far.

[tmiasko]

If we were comfortable with using intrinsics directly this seems like nice win, but otherwise I wouldn't consider it to merit an extra compiler work, except for continued efforts towards enabling MIR inlining by default.

The area that does need compiler work are intrinsic wrappers in stdarch where overhead in the range of hundreds basic blocks is typical, in some cases going up to thousands of unnecessary basic blocks.

[oli-obk]

The area that does need compiler work are intrinsic wrappers in stdarch where overhead in the range of hundreds basic blocks is typical, in some cases going up to thousands of unnecessary basic blocks.

Do you think the problem is that we build these blocks at all, or could we just run an early mir opt to inline all intrinsic wrappers?

[tmiasko]

Do you think the problem is that we build these blocks at all, or could we just run an early mir opt to inline all intrinsic wrappers?

For the details regarding stdarch situation see rust-lang/stdarch#248.

[oli-obk]

oof. that's a whole different situation imo. The stdarch situation is about large function bodies. Here we just have a trivial body forwarding to an intrinsic, so we could just as well have no body and make the function the intrinsic. Not sure how well that goes with them actually being intrinsics, but similar to the Box::new -> box optimization suggested on zulip, we could make mem::size_of a lang item and do some special magic on calls to it.

[tmiasko]

Anyway, since there are reservation about using intrinsics directly I think that answers question about this proposal.

(I feel you might have misinterpreted my tangential comment about stdarch. I wasn't commenting about similarities, quite the contrary. I think the wrappers in stdarch would benefit for extra compiler work to make those functions trivial, unlike the wrappers here which I don't think would).