Transforms match into an assignment statement #120614

DianQK · 2024-02-03T14:48:27Z

We should be able to do some similar transformations, like enum to enum.

r? mir-opt

saethlin · 2024-02-04T01:15:38Z

Transforms match into an assignment statement Fixes rust-lang#106459. We should be able to do some similar transformations, like `enum` to `enum`. r? mir-opt

bors · 2024-02-04T01:16:47Z

⌛ Trying commit 7a47635 with merge eea65f2...

bors · 2024-02-04T02:46:00Z

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

rust-timer · 2024-02-04T04:00:56Z

Finished benchmarking commit (eea65f2): comparison URL.

Overall result: no relevant changes - no 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.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	2.1%	[1.2%, 3.7%]	7
Regressions ❌ (secondary)	2.9%	[2.0%, 4.7%]	20
Improvements ✅ (primary)	-4.1%	[-5.0%, -3.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[-5.0%, 3.7%]	9

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.2%	[-7.2%, -5.2%]	6
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.0%	[0.0%, 0.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.0%, 0.0%]	5

Bootstrap: 659.957s -> 661.541s (0.24%)
Artifact size: 308.08 MiB -> 308.11 MiB (0.01%)

rustbot · 2024-02-06T14:45:03Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler/rustc_mir_transform/src/match_branches.rs

cjgillot · 2024-02-04T14:45:00Z

compiler/rustc_mir_transform/src/match_branches.rs

+}
+
+trait SimplifyMatch<'tcx> {
+    fn simplify(


Could you comment what this function does?

compiler/rustc_mir_transform/src/match_branches.rs

cjgillot · 2024-02-04T14:48:07Z

compiler/rustc_mir_transform/src/match_branches.rs

+        let def_id = body.source.def_id();
+        let param_env = tcx.param_env_reveal_all_normalized(def_id);
+
+        let bbs = body.basic_blocks.as_mut();


Calling as_mut() clears the CFG caches unconditionally. Is there a way to delay this invalidation?

I submitted a separate commit. I invalidate the cache only after modifying the CFG. If the cache is used later in a loop, we need to invalidate it after each modification.

With MirPatch we don't need to worry about this.

cjgillot · 2024-02-13T14:48:01Z

compiler/rustc_mir_transform/src/match_branches.rs

+///    goto -> bb5;
+/// }
+/// ```
+impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp {


IIUC, this transform is a strict superset of SimplifyToIf, isn't it? Can we only keep this one?

Most of their code can't be shared, and if they were put together, I'd be worried that it would be difficult to maintain later. A major difference is that the otherwise target of SimplifyToIf is reachable, while SimplifyToExp is unreachable.

bors · 2024-02-13T19:50:39Z

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

DianQK · 2024-02-21T11:44:56Z

Sorry for my late reply.
@rustbot ready

…tement as well

DianQK · 2024-04-08T11:28:00Z

Rebased. :>

WaffleLapkin · 2024-04-08T14:40:40Z

@bors r=cjgillot

bors · 2024-04-08T14:40:43Z

📌 Commit f440737 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-04-08T18:28:53Z

⌛ Testing commit f440737 with merge 211518e...

bors · 2024-04-08T20:30:01Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 211518e to master...

rust-timer · 2024-04-08T21:48:59Z

Finished benchmarking commit (211518e): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

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

Cycles

This benchmark run did not return any relevant results for this metric.

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

Bootstrap: 669.664s -> 668.886s (-0.12%)
Artifact size: 318.23 MiB -> 318.49 MiB (0.08%)

RalfJung · 2024-04-17T22:44:45Z

compiler/rustc_mir_transform/src/match_branches.rs

-            let (from, first, second) = bbs.pick3_mut(bb_idx, first, second);
+        fn int_equal(l: ScalarInt, r: impl Into<u128>, size: Size) -> bool {
+            l.try_to_int(l.size()).unwrap()
+                == ScalarInt::try_from_uint(r, size).unwrap().try_to_int(size).unwrap()


What is the point of doing the comparison in such a complicated way? Why turn r into a ScalarInt and then back into a i128?

Because SwitchTargets only saves the value of the corresponding bit value by u128, it lacks information on bit-width and sign.

It doesn't need the sign, as I said: == is sign-independent, so we can figure out the switch target without knowing the sign. And it has the width, it is determined by the type of the match operand.

Ah, you're right. This code is used to compare signed integers of different widths, such as i8 and i16. We must add additional conversions for this scenario.
Hmm, it's been just a month, and I have to carefully review the code to respond correctly. It seems I indeed wrote some hard-to-understand code. :>
I'll add some comments. ~~I should also move the unsigned comparison to the front, as I expect this might make the code a bit faster.~~

RalfJung · 2024-04-17T22:51:10Z

compiler/rustc_mir_transform/src/match_branches.rs

+                                && int_equal(s, second_val, discr_size))
+                                || (Some(f) == ScalarInt::try_from_uint(first_val, f.size())
+                                    && Some(s)
+                                        == ScalarInt::try_from_uint(second_val, s.size())) =>


This is very strange. == is sign-independent, so I don't understand why both cases need to be considered here. Furthermore, if the sign mattered, then surely it makes no sense to check they_are_equal_signed || they_are_equal_unsigned; instead you have to check if they_are_signed { they_are_equal_signed } else { they_are_equal_unsigned }. Finally, f_c.const_.ty().is_signed() || discr_ty.is_signed() sounds like you are mixing signed and unsigned values (as in, LHS and RHS can have different sign), which should never happen.

What is going on here?

This is used to handle conversions such as from enum(u32) to i32 or from enum(i32) to u32:

rust/tests/mir-opt/matches_reduce_branches.rs

Lines 69 to 85 in e752af7

#[repr(u8)]

enum EnumAu8 {

A = 1,

B = 2,

}

// EMIT_MIR matches_reduce_branches.match_u8_i16.MatchBranchSimplification.diff

fn match_u8_i16(i: EnumAu8) -> i16 {

// CHECK-LABEL: fn match_u8_i16(

// CHECK-NOT: switchInt

// CHECK: _0 = _3 as i16 (IntToInt);

// CHECH: return

match i {

EnumAu8::A => 1,

EnumAu8::B => 2,

}

}

.

Aha... I have no idea what this means. ;)

But it makes no sense to compare things twice here. == is entirely based on being equal bitwise, so the sign doesn't matter.

Same as above (different width), corresponding test case:

rust/tests/mir-opt/matches_reduce_branches.rs

Lines 197 to 215 in e752af7

#[repr(i8)]

enum EnumAi8 {

A = -1,

B = 2,

C = -3,

}

// EMIT_MIR matches_reduce_branches.match_i8_i16.MatchBranchSimplification.diff

fn match_i8_i16(i: EnumAi8) -> i16 {

// CHECK-LABEL: fn match_i8_i16(

// CHECK-NOT: switchInt

// CHECK: _0 = _3 as i16 (IntToInt);

// CHECH: return

match i {

EnumAi8::A => -1,

EnumAi8::B => 2,

EnumAi8::C => -3,

}

}

But you are comparing both signed and unsigned representation completely disregarding whether the value is actually signed or unsigned. It sounds like you want two cases

value is signed; then convert everything to i128 (with sign extension, e.g. via try_to_int) and compare there

value is unsigned; the convert everything to u128 and compare there

But currently you're interpreting the number both ways and then checking if either comparison succeeds. It seems to me that can sometimes lead to blatantly wrong results, e.g. when two numbers are equal unsigned but different after sign extension, and they are actually signed -- you code will treat them as equal, I think?

Ah, I'm fixing it right now.

Fixed in #124122, thanks for pointing it out.

rustbot assigned oli-obk Feb 3, 2024

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 Feb 3, 2024

This comment has been minimized.

Sign in to view

DianQK force-pushed the simplify-switch-int branch from f21eda9 to 7a47635 Compare February 4, 2024 00:10

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 Feb 4, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 4, 2024

cjgillot self-assigned this Feb 4, 2024

DianQK force-pushed the simplify-switch-int branch from 7a47635 to a578b6f Compare February 6, 2024 14:43

DianQK marked this pull request as ready for review February 6, 2024 14:45

DianQK mentioned this pull request Feb 7, 2024

Unnecesary discriminant checks in PartialEq/PartialOrd style code on enums #119014

Closed

cjgillot reviewed Feb 13, 2024

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 Feb 18, 2024

DianQK force-pushed the simplify-switch-int branch 2 times, most recently from 2c51ed1 to f79fd40 Compare February 20, 2024 14:21

oli-obk removed their assignment Feb 20, 2024

DianQK force-pushed the simplify-switch-int branch 2 times, most recently from dec8b9d to d47cf55 Compare February 21, 2024 10:54

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 Apr 7, 2024

DianQK force-pushed the simplify-switch-int branch from 755d9e5 to 6d577fc Compare April 8, 2024 10:48

DianQK added 5 commits April 8, 2024 18:54

Update matches_reduce_branches.rs

badb73b

Refactor MatchBranchSimplification

7af7458

Transforms match into an assignment statement

1f061f4

Transforms a match containing negative numbers into an assignment sta…

e752af7

…tement as well

Updating the MIR with MirPatch

254289a

This comment has been minimized.

Sign in to view

DianQK added 2 commits April 8, 2024 19:07

Add comments for CompareType

032bb74

Change the return type of can_simplify to Option<()>

f440737

DianQK force-pushed the simplify-switch-int branch from 6d577fc to f440737 Compare April 8, 2024 11:13

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 Apr 8, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2024

bors merged commit 211518e into rust-lang:master Apr 8, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 8, 2024

DianQK deleted the simplify-switch-int branch April 8, 2024 20:51

RalfJung reviewed Apr 17, 2024

View reviewed changes

DianQK mentioned this pull request Apr 18, 2024

Don't perform unsigned comparisons for signed integers #124122

Open

This was referenced Apr 19, 2024

Miscompilation due to MatchBranchSimplification MIR pass mixing up discriminants #124150

Closed

Disable SimplifyToExp in MatchBranchSimplification #124156

Merged

DianQK mentioned this pull request Apr 19, 2024

Use a known-working version of Rust nightly. ykjit/yk#1104

Merged

	#[repr(u8)]
	enum EnumAu8 {
	A = 1,
	B = 2,
	}

	// EMIT_MIR matches_reduce_branches.match_u8_i16.MatchBranchSimplification.diff
	fn match_u8_i16(i: EnumAu8) -> i16 {
	// CHECK-LABEL: fn match_u8_i16(
	// CHECK-NOT: switchInt
	// CHECK: _0 = _3 as i16 (IntToInt);
	// CHECH: return
	match i {
	EnumAu8::A => 1,
	EnumAu8::B => 2,
	}
	}

	#[repr(i8)]
	enum EnumAi8 {
	A = -1,
	B = 2,
	C = -3,
	}

	// EMIT_MIR matches_reduce_branches.match_i8_i16.MatchBranchSimplification.diff
	fn match_i8_i16(i: EnumAi8) -> i16 {
	// CHECK-LABEL: fn match_i8_i16(
	// CHECK-NOT: switchInt
	// CHECK: _0 = _3 as i16 (IntToInt);
	// CHECH: return
	match i {
	EnumAi8::A => -1,
	EnumAi8::B => 2,
	EnumAi8::C => -3,
	}
	}

Transforms match into an assignment statement #120614

Transforms match into an assignment statement #120614

Conversation

DianQK commented Feb 3, 2024

This comment has been minimized.

saethlin commented Feb 4, 2024

This comment has been minimized.

bors commented Feb 4, 2024

bors commented Feb 4, 2024

This comment has been minimized.

rust-timer commented Feb 4, 2024

Overall result: no relevant changes - no action needed

rustbot commented Feb 6, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 13, 2024

DianQK commented Feb 21, 2024 • edited

This comment has been minimized.

DianQK commented Apr 8, 2024

WaffleLapkin commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

bors commented Apr 8, 2024

rust-timer commented Apr 8, 2024

Overall result: ❌ regressions - no action needed

RalfJung Apr 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Apr 18, 2024 • edited

Choose a reason for hiding this comment

DianQK Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianQK Apr 18, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianQK commented Feb 21, 2024 •

edited

RalfJung Apr 17, 2024 •

edited

RalfJung Apr 18, 2024 •

edited

DianQK Apr 18, 2024 •

edited

DianQK Apr 18, 2024 •

edited