Simplify match based on the cast result of `IntToInt` #127324

DianQK · 2024-07-04T15:02:32Z

Continue to complete #124150. The condition in #120614 is wrong, e.g. -1i8 cannot be converted to 255i16. I've rethought the issue and simplified the conditional judgment for a more straightforward approach. The new approach is to check if the case value after the IntToInt conversion equals the target value.

In different types, IntToInt uses different casting methods. This rule is as follows:

i8/u8 to i8/u8: do nothing.
i8 to i16/u16: sign extension.
u8 to i16/u16: zero extension.
i16/u16 to i8/u8: truncate to the target size.

The previous error was a mix of zext and sext.

r? mir-opt

DianQK · 2024-07-04T15:03:30Z

@bors try @rust-timer queue

[WIP] Simplify match based on the cast result of `IntToInt` Even though the current method has become simpler and clearer, I should still add more comprehensive tests. r? ghost

bors · 2024-07-04T15:04:41Z

⌛ Trying commit faa5493 with merge b60a81d...

bors · 2024-07-04T16:47:40Z

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

rust-timer · 2024-07-04T21:40:27Z

Finished benchmarking commit (b60a81d): 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 (primary 2.6%, secondary -0.7%)

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.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-0.9%, -0.5%]	4
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

Cycles

Results (secondary -0.4%)

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

Binary size

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

Bootstrap: 734.204s -> 754.142s (2.72%)
Artifact size: 328.34 MiB -> 328.21 MiB (-0.04%)

DianQK · 2024-07-05T02:23:22Z

Sorry, bors, I forgot to enable it.

@bors try @rust-timer queue

bors · 2024-07-05T02:24:33Z

⌛ Trying commit 5bbd1f2 with merge ef695de...

[WIP] Simplify match based on the cast result of `IntToInt` Even though the current method has become simpler and clearer, I should still add more comprehensive tests. r? ghost

bors · 2024-07-05T04:06:49Z

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

rust-timer · 2024-07-05T05:29:06Z

Finished benchmarking commit (ef695de): 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 (primary -0.9%, secondary -2.0%)

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.6%	[1.9%, 3.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-7.8%	[-7.8%, -7.8%]	1
Improvements ✅ (secondary)	-2.0%	[-3.2%, -0.9%]	6
All ❌✅ (primary)	-0.9%	[-7.8%, 3.2%]	3

Cycles

Results (secondary 0.4%)

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

Binary size

Results (primary 0.0%)

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.2%	[0.1%, 0.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.3%, -0.0%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.3%, 0.4%]	9

Bootstrap: 699.885s -> 699.501s (-0.05%)
Artifact size: 328.23 MiB -> 328.37 MiB (0.04%)

rustbot · 2024-07-07T10:16:22Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

DianQK · 2024-07-07T10:13:05Z

tests/mir-opt/matches_reduce_branches.rs

@@ -248,7 +627,8 @@ enum EnumAi128 {
 // EMIT_MIR matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff
 fn match_i128_u128(i: EnumAi128) -> u128 {


I'm not sure if it's necessary to add i128 or u128 tests anymore.

DianQK · 2024-07-07T10:14:36Z

tests/mir-opt/matches_reduce_branches.rs

+        EnumAu16::u65280_0xff00 => 0,
+        EnumAu16::u65407_0xff7f => 127,
+        EnumAu16::u65408_0xff80 => 128,
+        EnumAu16::u65535_0xffff => 127, // failed


Truncation disregards the sign, so I don't have any good negative test cases. :(

cjgillot · 2024-07-07T11:23:43Z

compiler/rustc_mir_transform/src/match_branches.rs

+        }
+    };
+    cast_scalar == target_scalar
+}


This code does not exactly match what the interpreter does in cast_from_int_like. Should we reuse similar code?

Updated.
When I read this part of the code earlier, I tried to call it directly, which seemed a bit difficult. I copied part of the code. Maybe I should put such code in rustc_middle?

rustc_const_eval is accessible from this crate, so putting is there is enough.

Updated, I have squashed them into two commits.

tests/mir-opt/matches_reduce_branches.rs

rustbot · 2024-07-07T14:11:31Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

DianQK · 2024-07-22T01:21:47Z

ping? :)

saethlin · 2024-07-22T02:07:42Z

r? cjgillot
Feel free to send it back if you want, but it looks like you're already reviewing this.

bors · 2024-07-29T05:15:23Z

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

bors · 2024-08-01T05:01:18Z

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

cjgillot · 2024-08-03T15:49:04Z

@bors r+ rollup=never

bors · 2024-08-03T15:49:06Z

📌 Commit 1f9d960 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-08-03T18:48:33Z

⌛ Testing commit 1f9d960 with merge bbf60c8...

bors · 2024-08-03T21:13:32Z

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

rust-timer · 2024-08-03T23:15:01Z

Finished benchmarking commit (bbf60c8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary -1.9%)

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.0%	[5.0%, 5.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-8.7%	[-8.7%, -8.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.9%	[-8.7%, 5.0%]	2

Cycles

Results (primary 1.4%)

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

Binary size

Results (primary 0.0%)

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

Bootstrap: 760.453s -> 758.56s (-0.25%)
Artifact size: 336.89 MiB -> 336.83 MiB (-0.02%)

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

DianQK mentioned this pull request Jul 4, 2024

Don't perform unsigned comparisons for signed integers #124122

Closed

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

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 Jul 5, 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 Jul 5, 2024

DianQK force-pushed the match-br branch 2 times, most recently from 12e5738 to 2ae25d5 Compare July 7, 2024 07:27

rustbot assigned saethlin Jul 7, 2024

DianQK changed the title ~~[WIP] Simplify match based on the cast result of IntToInt~~ Simplify match based on the cast result of IntToInt Jul 7, 2024

DianQK marked this pull request as ready for review July 7, 2024 10:16

DianQK commented Jul 7, 2024

View reviewed changes

cjgillot reviewed Jul 7, 2024

View reviewed changes

rustbot assigned cjgillot and unassigned saethlin Jul 22, 2024

DianQK force-pushed the match-br branch 4 times, most recently from 8300db0 to 715558d Compare July 31, 2024 00:06

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the master branch from 2318850 to f8060d2 Compare July 31, 2024 00:11

DianQK force-pushed the match-br branch from 715558d to 10d9374 Compare July 31, 2024 00:14

DianQK added 2 commits August 3, 2024 10:55

Simplify match based on the cast result of IntToInt.

8b9d7b1

Re-enable SimplifyToExp in match_branches.

1f9d960

DianQK force-pushed the match-br branch from 10d9374 to 1f9d960 Compare August 3, 2024 02:58

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

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2024

bors merged commit bbf60c8 into rust-lang:master Aug 3, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 3, 2024

DianQK deleted the match-br branch August 3, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify match based on the cast result of `IntToInt` #127324

Simplify match based on the cast result of `IntToInt` #127324

DianQK commented Jul 4, 2024 •

edited

Loading

DianQK commented Jul 4, 2024

This comment has been minimized.

bors commented Jul 4, 2024

This comment has been minimized.

bors commented Jul 4, 2024

This comment has been minimized.

rust-timer commented Jul 4, 2024

DianQK commented Jul 5, 2024

This comment has been minimized.

bors commented Jul 5, 2024

bors commented Jul 5, 2024

This comment has been minimized.

rust-timer commented Jul 5, 2024

rustbot commented Jul 7, 2024

DianQK Jul 7, 2024

DianQK Jul 7, 2024

cjgillot Jul 7, 2024

DianQK Jul 7, 2024

cjgillot Jul 7, 2024

DianQK Jul 31, 2024

rustbot commented Jul 7, 2024

DianQK commented Jul 22, 2024

saethlin commented Jul 22, 2024

bors commented Jul 29, 2024

This comment has been minimized.

bors commented Aug 1, 2024

cjgillot commented Aug 3, 2024

bors commented Aug 3, 2024

bors commented Aug 3, 2024

bors commented Aug 3, 2024

rust-timer commented Aug 3, 2024

		@@ -248,7 +627,8 @@ enum EnumAi128 {
		// EMIT_MIR matches_reduce_branches.match_i128_u128.MatchBranchSimplification.diff
		fn match_i128_u128(i: EnumAi128) -> u128 {

Simplify match based on the cast result of IntToInt #127324

Simplify match based on the cast result of IntToInt #127324

Conversation

DianQK commented Jul 4, 2024 • edited Loading

DianQK commented Jul 4, 2024

This comment has been minimized.

bors commented Jul 4, 2024

This comment has been minimized.

bors commented Jul 4, 2024

This comment has been minimized.

rust-timer commented Jul 4, 2024

Overall result: no relevant changes - no action needed

DianQK commented Jul 5, 2024

This comment has been minimized.

bors commented Jul 5, 2024

bors commented Jul 5, 2024

This comment has been minimized.

rust-timer commented Jul 5, 2024

Overall result: no relevant changes - no action needed

rustbot commented Jul 7, 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

rustbot commented Jul 7, 2024

DianQK commented Jul 22, 2024

saethlin commented Jul 22, 2024

bors commented Jul 29, 2024

This comment has been minimized.

bors commented Aug 1, 2024

cjgillot commented Aug 3, 2024

bors commented Aug 3, 2024

bors commented Aug 3, 2024

bors commented Aug 3, 2024

rust-timer commented Aug 3, 2024

Overall result: no relevant changes - no action needed

Simplify match based on the cast result of `IntToInt` #127324

Simplify match based on the cast result of `IntToInt` #127324

DianQK commented Jul 4, 2024 •

edited

Loading