Only unpack tupled args in inliner if we expect args to be unpacked #110833

compiler-errors · 2023-04-25T22:23:20Z

"rust-call" is a strange function abi. sometimes, it expects the arguments to be unpacked by the caller and passed as individual args (closure bodies), and sometimes it does not (user functions annotated with the "rust-call" abi).

make sure the mir inliner respects this difference when checking that arguments are compatible, and doesn't try to ICE when we call a extern "rust-call" function in a generic context.

fixes #110829

rustbot · 2023-04-25T22:23:26Z

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-04-25T22:23:28Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2023-04-25T22:23:32Z

r? @cjgillot

compiler-errors · 2023-04-26T01:17:03Z

Oh hey, also turns out that this allows us to inline <Box<dyn Fn()> as Fn<()>>::call which we weren't doing that before...

Maybe worth running perf idk

@bors try @rust-timer queue

bors · 2023-04-26T01:17:12Z

⌛ Trying commit e8027562f19ae973eabc1a217c51ee965fcd9fce with merge 28bd1d3010201044fa44005fae674a82a8868cbf...

bors · 2023-04-26T02:59:05Z

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

rust-timer · 2023-04-26T04:28:33Z

Finished benchmarking commit (28bd1d3010201044fa44005fae674a82a8868cbf): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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)	2.4%	[0.3%, 62.9%]	36
Regressions ❌ (secondary)	0.4%	[0.3%, 0.6%]	14
Improvements ✅ (primary)	-1.1%	[-1.8%, -0.4%]	2
Improvements ✅ (secondary)	-1.3%	[-1.6%, -1.2%]	3
All ❌✅ (primary)	2.3%	[-1.8%, 62.9%]	38

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)	4.0%	[3.3%, 4.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-3.9%, -0.1%]	4
Improvements ✅ (secondary)	-3.0%	[-3.6%, -2.3%]	3
All ❌✅ (primary)	0.4%	[-3.9%, 4.6%]	6

Cycles

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)	23.9%	[1.5%, 64.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.8%	[-1.8%, -1.8%]	1
Improvements ✅ (secondary)	-1.4%	[-1.6%, -1.2%]	3
All ❌✅ (primary)	17.4%	[-1.8%, 64.0%]	4

compiler-errors · 2023-04-26T05:16:38Z

lol, lmao

compiler-errors · 2023-04-26T05:59:05Z

Well this just seems to be a consequence of fixing the MIR inliner to actually work on extern "rust-call" functions.

Not exactly sure how to proceed, other than to make the inliner just ignore non-closure extern "rust-call" functions or something like that.

cjgillot · 2023-04-26T19:56:18Z

tests/mir-opt/inline/inline_box_fn.rs

@@ -0,0 +1,6 @@
+// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib


Suggested change

// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib

// unit-test: Inline

// compile-flags: --crate-type=lib

cjgillot · 2023-04-26T19:58:24Z

tests/mir-opt/inline/dont_ice_on_generic_rust_call.rs

@@ -0,0 +1,10 @@
+// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib


Suggested change

// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib

// unit-test: Inline

// compile-flags: --crate-type=lib

cjgillot · 2023-04-30T10:25:14Z

compiler/rustc_mir_transform/src/inline.rs

+                };
+                arg_tuple_tys.as_slice()
+            } else {
+                std::slice::from_ref(&arg_tuple_ty)
            };


TBH, this whole branch about the RustCall ABI looks wrong. Should it mirror make_call_args, and return Err instead of bug! in error cases?

I mean: the previous implementation already looked fishy 😄

cjgillot · 2023-04-30T10:27:33Z

Any idea what extra functions are getting inlined?

compiler-errors · 2023-05-08T04:10:26Z

Sorry for the late response --

Any idea what extra functions are getting inlined?

Not exactly sure, but I'm guessing it's the calls to Box's implementations of Fn family traits, or maybe the blanket impls that we have for &F and &mut F...?

compiler-errors · 2023-07-22T16:21:34Z

wtf, x test mir-opt and x test tests/mir-opt do different things 🤦

compiler-errors · 2023-07-31T01:53:26Z

whoops, forgot to r= this

@bors r=cjgillot

bors · 2023-07-31T01:53:28Z

📌 Commit 21fbedfbc51941c31a963269ed28458a04524fc4 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-07-31T04:21:16Z

⌛ Testing commit 21fbedfbc51941c31a963269ed28458a04524fc4 with merge 499232bee22d8d4be92c641b9f352625edb3a607...

bors · 2023-07-31T04:40:31Z

💔 Test failed - checks-actions

xiaoyawei · 2023-08-03T07:17:07Z

really looking forwarding to seeing this PR merged :)

compiler-errors · 2023-08-03T19:52:10Z

Had to use // EMIT_MIR_FOR_EACH_PANIC_STRATEGY, that should fix it.

@bors r=cjgillot

bors · 2023-08-03T19:52:12Z

📌 Commit e43649f has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-08-04T00:45:28Z

⌛ Testing commit e43649f with merge 60fa393...

bors · 2023-08-04T02:35:40Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 60fa393 to master...

rust-timer · 2023-08-04T07:54:00Z

Finished benchmarking commit (60fa393): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -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.2%	[0.2%, 0.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	2

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)	3.5%	[2.6%, 4.3%]	2
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-1.8%	[-2.7%, -0.9%]	3
Improvements ✅ (secondary)	-2.6%	[-3.4%, -1.6%]	3
All ❌✅ (primary)	0.3%	[-2.7%, 4.3%]	5

Cycles

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)	3.2%	[1.9%, 4.4%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

Bootstrap: 648.71s -> 649.049s (0.05%)

rustbot assigned b-naber Apr 25, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 25, 2023

rustbot assigned cjgillot and unassigned b-naber Apr 25, 2023

rust-cloud-vms bot force-pushed the rustc-call-inliner-ice branch from 00531e6 to e802756 Compare April 26, 2023 01:16

This comment has been minimized.

Sign in to view

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

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 26, 2023

This comment was marked as duplicate.

Sign in to view

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

This comment was marked as duplicate.

Sign in to view

compiler-errors removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 28, 2023

cjgillot reviewed Apr 30, 2023

View reviewed changes

cjgillot 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 15, 2023

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023

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 Jul 31, 2023

lqd mentioned this pull request Aug 2, 2023

cannot compile serde_traitobject #114361

Closed

compiler-errors added 4 commits August 3, 2023 18:35

Negative test for inlining <Box<dyn Fn()> as Fn<()>>::call

2fd4c28

Only unpack tupled args in inliner if we expect args to be unpacked

0391af0

Explicitly don't inline user-written rust-call fns

3c9549b

bless

e43649f

rust-cloud-vms bot force-pushed the rustc-call-inliner-ice branch from 21fbedf to e43649f Compare August 3, 2023 18:42

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2023

bors merged commit 60fa393 into rust-lang:master Aug 4, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Aug 4, 2023

compiler-errors deleted the rustc-call-inliner-ice branch August 11, 2023 20:19

CosmicHorrorDev mentioned this pull request Oct 3, 2023

Can't compile with latest nightly release CodeSandwich/Mocktopus#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only unpack tupled args in inliner if we expect args to be unpacked #110833

Only unpack tupled args in inliner if we expect args to be unpacked #110833

compiler-errors commented Apr 25, 2023 •

edited

Loading

rustbot commented Apr 25, 2023

rustbot commented Apr 25, 2023

compiler-errors commented Apr 25, 2023

compiler-errors commented Apr 26, 2023 •

edited

Loading

This comment has been minimized.

bors commented Apr 26, 2023

bors commented Apr 26, 2023

This comment has been minimized.

rust-timer commented Apr 26, 2023

compiler-errors commented Apr 26, 2023

compiler-errors commented Apr 26, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

cjgillot Apr 26, 2023

cjgillot Apr 26, 2023

cjgillot Apr 30, 2023

cjgillot Apr 30, 2023

cjgillot commented Apr 30, 2023

compiler-errors commented May 8, 2023

compiler-errors commented Jul 22, 2023

compiler-errors commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

This comment has been minimized.

bors commented Jul 31, 2023

xiaoyawei commented Aug 3, 2023

compiler-errors commented Aug 3, 2023

bors commented Aug 3, 2023

bors commented Aug 4, 2023

bors commented Aug 4, 2023

rust-timer commented Aug 4, 2023

		@@ -0,0 +1,6 @@
		// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib

	// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib
	// unit-test: Inline
	// compile-flags: --crate-type=lib

		@@ -0,0 +1,10 @@
		// compile-flags: -Zmir-enable-passes=+Inline --crate-type=lib

Only unpack tupled args in inliner if we expect args to be unpacked #110833

Only unpack tupled args in inliner if we expect args to be unpacked #110833

Conversation

compiler-errors commented Apr 25, 2023 • edited Loading

rustbot commented Apr 25, 2023

rustbot commented Apr 25, 2023

compiler-errors commented Apr 25, 2023

compiler-errors commented Apr 26, 2023 • edited Loading

This comment has been minimized.

bors commented Apr 26, 2023

bors commented Apr 26, 2023

This comment has been minimized.

rust-timer commented Apr 26, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

compiler-errors commented Apr 26, 2023

compiler-errors commented Apr 26, 2023

This comment was marked as duplicate.

This comment was marked as duplicate.

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

cjgillot commented Apr 30, 2023

compiler-errors commented May 8, 2023

compiler-errors commented Jul 22, 2023

compiler-errors commented Jul 31, 2023

bors commented Jul 31, 2023

bors commented Jul 31, 2023

This comment has been minimized.

bors commented Jul 31, 2023

xiaoyawei commented Aug 3, 2023

compiler-errors commented Aug 3, 2023

bors commented Aug 3, 2023

bors commented Aug 4, 2023

bors commented Aug 4, 2023

rust-timer commented Aug 4, 2023

Overall result: ❌✅ regressions and improvements - no action needed

compiler-errors commented Apr 25, 2023 •

edited

Loading

compiler-errors commented Apr 26, 2023 •

edited

Loading