Refactor call terminator to always include destination place #96098

JakobDegen · 2022-04-16T00:53:00Z

In #71117 people seemed to agree that call terminators should always have a destination place, even if the call was guaranteed to diverge. This implements that. Unsurprisingly, the diff touches a lot of code, but thankfully I had to do almost nothing interesting. The only interesting thing came up in const prop, where the stack frame having no return place was also used to indicate that the layout could not be computed (or similar). I replaced this with a ZST allocation, which should continue to do the right things.

cc @RalfJung @eddyb who were involved in the original conversation

r? rust-lang/mir-opt

rust-highfive · 2022-04-16T00:53:03Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

nagisa · 2022-04-16T00:58:04Z

@bors try @rust-timer queue

rust-timer · 2022-04-16T00:58:06Z

Awaiting bors try build completion.

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

bors · 2022-04-16T00:58:13Z

⌛ Trying commit 303e3a4388f8ef0df607abbd8f0d4b8fe23e3c27 with merge d584f7c49e7118da3500a697b2f6e3ba36f4af0a...

bors · 2022-04-16T02:19:09Z

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

rust-timer · 2022-04-16T02:19:11Z

Queued d584f7c49e7118da3500a697b2f6e3ba36f4af0a with parent 3f391b8, future comparison URL.

rust-timer · 2022-04-16T04:27:10Z

Finished benchmarking commit (d584f7c49e7118da3500a697b2f6e3ba36f4af0a): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	11	2	15	2
mean²	N/A	0.5%	-0.3%	-0.5%	-0.3%
max	N/A	1.1%	-0.4%	-1.2%	-0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

oli-obk · 2022-04-16T05:58:56Z

Small regression in const eval likely just llvm noise around the const eval logic, but please check by running the valgrind command from the link above

nagisa

It is pretty straightforward-looking change.

nagisa · 2022-04-16T10:52:41Z

compiler/rustc_middle/src/mir/terminator.rs

+        /// Where the returned value will be written
+        dest_place: Place<'tcx>,
+        /// Where to go after this call returns. If none, the call necessarily diverges.
+        dest_block: Option<BasicBlock>,


IMO there's little value in making this optional if the place isn’t. If the function call is guaranteed to not return, we can just codegen dest_block into just an unreachable. Arguably that will serve to homogenize semantics of MIR (and analysis) further.

Hmm, I'm not sure. Intuitively, it feels like this information being accessible without having to go looking further might be valuable and simplify things. Maybe not though. I also don't know which version would be better for perf.

In any case, if we do consider such a change, I think we should make a uniform decision for this, TerminatorKind::SwitchInt (which currently requires an otherwise block), and TerminatorKind::InlineAsm (which does not require a destination/target block) on the basis of the needs of all three. That seems like too big/complicated of a conversation to have for this PR.

agreed, let's try this in a follow up

compiler/rustc_middle/src/mir/terminator.rs

compiler/rustc_codegen_cranelift/src/base.rs

RalfJung · 2022-04-16T13:06:21Z

compiler/rustc_const_eval/src/interpret/terminator.rs

+                    extra,
+                    caller_abi,
+                    args,
+                    dest_block.map(|x| (dest_place, x)),


Looks like we should propagate this change through the machine interface all the way to Miri?

(Same for M::call_intrinsic and M::find_mir_or_eval_fn.)

Yeah, I had avoided doing that for now because I'm not sure what the right procedure for it is. Should I just make the change and then send a follow-up PR to miri?

Yes. It is okay for Miri to temporarily break in the rustc repo.

Ideally you can prepare the Miri PR already before this one lands, so that once this one lands we can get Miri fixed ASAP.

JakobDegen · 2022-04-16T15:21:55Z

I only half know how to read these things, but nothing in the diff reported by cachegrind stands out:

Diff

--------------------------------------------------------------------------------
Files compared:   results/cgfilt-3f391b84552f210adec7893b50c5da74f9362ae4-ctfe-stress-5-Debug-IncrFull; results/cgfilt-d584f7c49e7118da3500a697b2f6e3ba36f4af0a-ctfe-stress-5-Debug-IncrFull
Command:          /home/jake/.rustup/toolchains/3f391b84552f210adec7893b50c5da74f9362ae4/bin/rustc --crate-name ctfe_stress_5 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 -C incremental=/tmp/.tmpfvxNSK/incremental-state -C metadata=4aff8ac78ce36ed0 -C extra-filename=-4aff8ac78ce36ed0 --out-dir /tmp/.tmpfvxNSK/target/debug/deps -L dependency=/tmp/.tmpfvxNSK/target/debug/deps -Adeprecated -Aunknown-lints -Zincremental-verify-ich; /home/jake/.rustup/toolchains/d584f7c49e7118da3500a697b2f6e3ba36f4af0a/bin/rustc --crate-name ctfe_stress_5 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no -C debuginfo=2 -C incremental=/tmp/.tmpM6OXv8/incremental-state -C metadata=4aff8ac78ce36ed0 -C extra-filename=-4aff8ac78ce36ed0 --out-dir /tmp/.tmpM6OXv8/target/debug/deps -L dependency=/tmp/.tmpM6OXv8/target/debug/deps -Adeprecated -Aunknown-lints -Zincremental-verify-ich
Data file:        results/cgfilt-diff-3f391b84552f210adec7893b50c5da74f9362ae4-d584f7c49e7118da3500a697b2f6e3ba36f4af0a-ctfe-stress-5-Debug-IncrFull
Events recorded:  Ir
Events shown:     Ir
Event sort order: Ir
Thresholds:       0.1
Include dirs:     
User annotated:   
Auto-annotation:  on

--------------------------------------------------------------------------------
Ir          
--------------------------------------------------------------------------------
126,984,827  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir           file:function
--------------------------------------------------------------------------------
 77,782,066  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::run
 40,129,096  ???:__memcpy_sse2_unaligned_erms
 27,525,798  ???:<rustc_data_structures::intern::Interned<rustc_middle::mir::interpret::allocation::Allocation> as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable
-11,011,547  ???:__memcpy_avx_unaligned_erms
 -6,235,908  ???:<rustc_const_eval::const_eval::machine::CompileTimeInterpreter as rustc_const_eval::interpret::machine::Machine>::find_mir_or_eval_fn
  5,141,834  ???:memcpy@GLIBC_2.2.5
 -4,470,840  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::push_stack_frame
 -2,202,162  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::eval_fn_call
  1,574,512  ???:llvm::MCAssembler::writeSectionData(llvm::raw_ostream&, llvm::MCSection const*, llvm::MCAsmLayout const&) const
 -1,153,476  ???:<rustc_const_eval::interpret::memory::AllocRef<rustc_middle::mir::interpret::AllocId, ()>>::read_ptr_sized
 -1,101,050  ???:core::iter::adapters::try_process::<core::iter::adapters::map::Map<core::slice::iter::Iter<rustc_middle::mir::Operand>, <rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::eval_operands::{closure
   -787,282  ???:<rustc_const_eval::interpret::memory::AllocRef<rustc_middle::mir::interpret::AllocId, ()>>::read_scalar
    688,131  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::operand_projection
   -486,026  ???:rustc_infer::infer::lexical_region_resolve::resolve
    433,001  ???:<rustc_infer::infer::lexical_region_resolve::LexicalResolver>::infer_variable_values
    367,016  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::mir_const_to_op
    139,917  ???:<rustc_middle::ty::instance::Instance>::resolve_opt_const_arg
    131,146  ???:<rustc_target::abi::TyAndLayout<rustc_middle::ty::Ty>>::field::<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>
    131,071  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::unsize_into_ptr

JakobDegen · 2022-04-16T15:22:18Z

@rustbot ready

bors · 2022-04-20T21:45:19Z

☔ The latest upstream changes (presumably #96253) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2022-05-23T10:27:27Z

⌛ Testing commit c8c64d3dbb7db11a37c9582f76bc68639543359a with merge 12da966580a170ebc534cf139395c0b9ba93baa5...

bors · 2022-05-23T10:46:44Z

💔 Test failed - checks-actions

JakobDegen · 2022-05-23T21:49:48Z

Required a rebase to show up locally, trivial fix

RalfJung · 2022-05-24T06:07:33Z

@bors r=oli-obk

bors · 2022-05-24T06:07:35Z

📌 Commit 09b0936 has been approved by oli-obk

bors · 2022-05-24T07:13:29Z

⌛ Testing commit 09b0936 with merge 43d9f38...

bors · 2022-05-24T09:54:03Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 43d9f38 to master...

rust-highfive · 2022-05-24T09:54:30Z

📣 Toolstate changed by #96098!

Tested on commit 43d9f38.
Direct link to PR: #96098

💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

@eddyb

Tested on commit rust-lang/rust@43d9f38. Direct link to PR: <rust-lang/rust#96098> 💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk). 💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

rust-timer · 2022-05-24T11:37:26Z

Finished benchmarking commit (43d9f38): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	0	5	17	5
mean²	N/A	N/A	-0.3%	-0.5%	-0.3%
max	N/A	N/A	-0.4%	-1.1%	-0.4%

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	1	1	0	2	1
mean²	3.6%	3.7%	N/A	-2.3%	3.6%
max	3.6%	3.7%	N/A	-2.3%	3.6%

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regression found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	1	0	0	0
mean²	N/A	2.7%	N/A	N/A	N/A
max	N/A	2.7%	N/A	N/A	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩ ↩² ↩³
the arithmetic mean of the percent change ↩ ↩² ↩³

Adjust Miri to also require return places everywhere This is the miri side of rust-lang/rust#96098 . It'll still need a bump to rust-version once the rust PR is merged, but the test suite passes against my local build of rustc.

…-obk Refactor call terminator to always include destination place In rust-lang#71117 people seemed to agree that call terminators should always have a destination place, even if the call was guaranteed to diverge. This implements that. Unsurprisingly, the diff touches a lot of code, but thankfully I had to do almost nothing interesting. The only interesting thing came up in const prop, where the stack frame having no return place was also used to indicate that the layout could not be computed (or similar). I replaced this with a ZST allocation, which should continue to do the right things. cc `@RalfJung` `@eddyb` who were involved in the original conversation r? rust-lang/mir-opt

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 16, 2022

rust-highfive assigned nagisa Apr 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 16, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 16, 2022

nagisa reviewed Apr 16, 2022

View reviewed changes

nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2022

bjorn3 reviewed Apr 16, 2022

View reviewed changes

compiler/rustc_codegen_cranelift/src/base.rs Outdated Show resolved Hide resolved

RalfJung reviewed Apr 16, 2022

View reviewed changes

JakobDegen force-pushed the always-return-place branch from 303e3a4 to b3f5015 Compare April 16, 2022 13:28

This comment has been minimized.

Sign in to view

JakobDegen force-pushed the always-return-place branch 2 times, most recently from 4f50b06 to cd38f83 Compare April 16, 2022 15:18

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2022

JakobDegen force-pushed the always-return-place branch from cd38f83 to c58d924 Compare April 22, 2022 20:30

This comment has been minimized.

Sign in to view

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 23, 2022

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 23, 2022

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2022

Refactor call terminator to always hold a destination place

09b0936

JakobDegen force-pushed the always-return-place branch from c8c64d3 to 09b0936 Compare May 23, 2022 21:49

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 24, 2022

bors merged commit 43d9f38 into rust-lang:master May 24, 2022

rustbot added this to the 1.63.0 milestone May 24, 2022

rust-highfive mentioned this pull request May 24, 2022

miri no longer builds after rust-lang/rust#96098 #97348

Closed

This was referenced May 24, 2022

Split dead store elimination off dest prop #97158

Merged

Add suggestion for relaxing static lifetime bounds on dyn trait impls in NLL #97284

Merged

JakobDegen deleted the always-return-place branch May 28, 2022 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor call terminator to always include destination place #96098

Refactor call terminator to always include destination place #96098

JakobDegen commented Apr 16, 2022

rust-highfive commented Apr 16, 2022

nagisa commented Apr 16, 2022

rust-timer commented Apr 16, 2022

bors commented Apr 16, 2022

bors commented Apr 16, 2022

rust-timer commented Apr 16, 2022

rust-timer commented Apr 16, 2022

oli-obk commented Apr 16, 2022

nagisa left a comment •

edited

Loading

nagisa Apr 16, 2022

JakobDegen Apr 16, 2022

oli-obk May 16, 2022

RalfJung Apr 16, 2022 •

edited

Loading

JakobDegen Apr 16, 2022

RalfJung Apr 16, 2022

This comment has been minimized.

JakobDegen commented Apr 16, 2022

JakobDegen commented Apr 16, 2022

bors commented Apr 20, 2022

This comment has been minimized.

bors commented May 23, 2022

This comment has been minimized.

bors commented May 23, 2022

JakobDegen commented May 23, 2022

RalfJung commented May 24, 2022

bors commented May 24, 2022

bors commented May 24, 2022

bors commented May 24, 2022

rust-highfive commented May 24, 2022

rust-timer commented May 24, 2022

Refactor call terminator to always include destination place #96098

Refactor call terminator to always include destination place #96098

Conversation

JakobDegen commented Apr 16, 2022

rust-highfive commented Apr 16, 2022

nagisa commented Apr 16, 2022

rust-timer commented Apr 16, 2022

bors commented Apr 16, 2022

bors commented Apr 16, 2022

rust-timer commented Apr 16, 2022

rust-timer commented Apr 16, 2022

Footnotes

oli-obk commented Apr 16, 2022

nagisa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Apr 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

JakobDegen commented Apr 16, 2022

JakobDegen commented Apr 16, 2022

bors commented Apr 20, 2022

This comment has been minimized.

bors commented May 23, 2022

This comment has been minimized.

bors commented May 23, 2022

JakobDegen commented May 23, 2022

RalfJung commented May 24, 2022

bors commented May 24, 2022

bors commented May 24, 2022

bors commented May 24, 2022

rust-highfive commented May 24, 2022

rust-timer commented May 24, 2022

Footnotes

nagisa left a comment •

edited

Loading

RalfJung Apr 16, 2022 •

edited

Loading