Don't perform unsigned comparisons for signed integers #124122

DianQK · 2024-04-18T15:15:10Z

Fixes (partial) #124150. (There are still some potential miscompilation with unsigned and signed integer transformation.)

Fixes a mis-compilation mentioned in #120614 (comment). We must handle signed and unsigned comparisons separately. We cannot cast -1i8 to 255i16.

This PR breaks the test case for unsigned and signed transformation, but I think this could go into a separate PR.

r? RalfJung or mir-opt

rustbot · 2024-04-18T15:15:19Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2024-04-18T17:26:24Z

compiler/rustc_mir_transform/src/match_branches.rs

@@ -399,7 +401,10 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp {
                            if ((f_c.const_.ty().is_signed() || discr_ty.is_signed())


Suggested change

if ((f_c.const_.ty().is_signed() || discr_ty.is_signed())

if ((f_c.const_.ty().is_signed() && discr_ty.is_signed())

Otherwise you're still potentially treating something as signed that is unsigned, or vice versa.

Honestly I think it's best to move this entire thing into a helper, not just int_equal.

And also, why is int_equal so different from what happens for unsigned? They should be entirely identical except for using int vs uint functions.

Ah I see, it's about converting the u128 from the SwitchInt to a ScalarInt. But that should be uniform -- do it once before the comparison. The input is not sign extended so it's always SwitchInt::try_from_uint.

So already further up, you should convert first_val and second_val to ScalarInt.

In fact we should probably change SwitchInt to store a ScalarInt rather than a raw u128, or at least make it easy to get the SwitchTarget values as ScalarInt.

Hold on a sec... isn't what you actually want to do here some sort of cast? I don't know the right direction, but -- basically you want to cast the discriminant value to the type of the constant (or the other way around), and then check they are equal, right?

The interpreter has the int_to_int_or_float method for that. But really you just care about this match arm. That should probably be turned into a helper somewhere it can be used by mir-opts.

Otherwise you're still potentially treating something as signed that is unsigned, or vice versa.

Everything looks fine here. Any signed integer will be converted to a signed comparison.

In fact we should probably change SwitchInt to store a ScalarInt rather than a raw u128, or at least make it easy to get the SwitchTarget values as ScalarInt.

I have seen your new issue. :)

The interpreter has the int_to_int_or_float method for that. But really you just care about this match arm. That should probably be turned into a helper somewhere it can be used by mir-opts.

Perhaps this could be a separate PR?

Everything looks fine here. Any signed integer will be converted to a signed comparison.

You're treating both values as signed if either of them is signed. That means you can be treating unsigned values as signed, which is wrong.

Perhaps this could be a separate PR?

Perhaps, but I don't understand you current PR, so it may also be a way to turn this code into something that makes sense to more than one person. ;)

Feel free to pick a different reviewer, but I can't make sense of what this code is trying to achieve. The comments don't explain the high-level picture (what are we even trying to achieve with this complicated series of checks) and the low-level details are clearly still mixing up signedness.

Everything looks fine here. Any signed integer will be converted to a signed comparison.

You're treating both values as signed if either of them is signed. That means you can be treating unsigned values as signed, which is wrong.

In the known test cases, it is correct. But I must carefully check the edge cases here. For safety reasons, I will later consider only signed-to-signed conversions in this PR. :)

cc @rust-lang/wg-mir-opt Perhaps someone else will directly point out this specific problem?

In the known test cases, it is correct.

That's a very low bar. The comments should give convincing reasons why it is correct for all possible MIR ever.

tests/mir-opt/matches_reduce_branches.rs

Co-authored-by: Ralf Jung <post@ralfj.de>

RalfJung · 2024-04-19T06:06:20Z

compiler/rustc_mir_transform/src/match_branches.rs

@@ -368,6 +368,8 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp {
            return None;
        }


Why does it make sense to compare the lengths of the basic blocks...?!?

Because we are currently only merging simple BBs, we have not considered BBs that could potentially be merged even though they have different numbers of instructions.

What's a "simple BB"? If both BBs have 24 instructions, how is that okay but one having 20 and the other 24 is not? The same number of instructions tells you absolutely nothing about what the BBs are doing...

There needs to be a comment here, at first sight this seems entirely nonsensical.

Sorry for the unclear expression. I mean the merging of basic blocks that only consider simple scenarios.
This is just an early bail-out. We assume that BBs with different the lengths of BBs cannot be merged.

RalfJung · 2024-04-19T06:11:11Z

Sorry, I don't have time to figure out how this optimization is supposed to behave, which is clearly required to be able to review this. This code is not clear enough to do review based on just a local diff, it needs someone to holistically understand the entire thing. And the first version of this optimization was unsound already, so maybe we should start by turning this into something obviously correct (e.g. full equality of the relevant values, no casting between different bitwdiths) and then slowly and systematically extend to more cases while adding comments that explain why this is actually correct. The invariants involved here are non-trivial and not reflected in the type system (because reflecting program equivalence in the type system is not possible in Rust^^).

MIR optimizations are among the most subtle code we have in rustc (in the sense that even if the code type-checks and does not panic, it is still easy to introduce very subtle bugs), they need to be treated with the utmost care.

r? mir-opt

DianQK · 2024-04-19T06:25:10Z

It's very interesting that you mentioned not understanding these codes, yet you pointed out the issues here from the code review. It's like one of my classmates scored full marks but told me that he actually didn't understand the exam questions. :3

RalfJung · 2024-04-19T09:00:37Z

I understand enough to find problems, not enough to be certain in its correctness. :)

apiraino · 2024-04-19T09:22:33Z

Folks: if T-compiler can be of any support in reviewing this, please don't hesitate to nominate for discussion.

Thanks for working on this!

Disable MatchBranchSimplification Due to the miscompilation mentioned in rust-lang#124150, We need to disable MatchBranchSimplification temporarily. To fully resolve this issue, my plan is: 1. Disable MatchBranchSimplification (this PR). 2. Remove all potentially unclear transforms in rust-lang#124122. 3. Gradually add back the removed transforms (possibly multiple PRs). r? `@Nilstrieb` or `@oli-obk`

…fJung Disable SimplifyToExp in MatchBranchSimplification Due to the miscompilation mentioned in rust-lang#124150, We need to disable MatchBranchSimplification temporarily. To fully resolve this issue, my plan is: 1. Disable SimplifyToExp in MatchBranchSimplification (this PR). 2. Remove all potentially unclear transforms in rust-lang#124122. 3. Gradually add back the removed transforms (possibly multiple PRs). r? `@Nilstrieb` or `@oli-obk`

DianQK added 2 commits April 18, 2024 23:11

Don't perform unsigned comparisons for signed integers

d0669a4

Add comments for int_equal

b894e53

rustbot assigned RalfJung Apr 18, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2024

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

DianQK mentioned this pull request Apr 18, 2024

Transforms match into an assignment statement #120614

Merged

RalfJung reviewed Apr 18, 2024

View reviewed changes

tests/mir-opt/matches_reduce_branches.rs Outdated Show resolved Hide resolved

Explain what happens in match_i8_i16_failed_2_a

f38b16e

Co-authored-by: Ralf Jung <post@ralfj.de>

RalfJung reviewed Apr 19, 2024

View reviewed changes

rustbot assigned wesleywiser and unassigned RalfJung Apr 19, 2024

RalfJung mentioned this pull request Apr 19, 2024

Miscompilation due to MatchBranchSimplification MIR pass mixing up discriminants #124150

Closed

DianQK mentioned this pull request Apr 19, 2024

Disable SimplifyToExp in MatchBranchSimplification #124156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't perform unsigned comparisons for signed integers #124122

Don't perform unsigned comparisons for signed integers #124122

DianQK commented Apr 18, 2024 •

edited

rustbot commented Apr 18, 2024

RalfJung Apr 18, 2024 •

edited

RalfJung Apr 18, 2024

RalfJung Apr 18, 2024 •

edited

RalfJung Apr 18, 2024

DianQK Apr 18, 2024

DianQK Apr 18, 2024

DianQK Apr 18, 2024

RalfJung Apr 19, 2024 •

edited

DianQK Apr 19, 2024

RalfJung Apr 19, 2024

RalfJung Apr 19, 2024

DianQK Apr 19, 2024

RalfJung Apr 19, 2024 •

edited

DianQK Apr 19, 2024

RalfJung commented Apr 19, 2024 •

edited

DianQK commented Apr 19, 2024

RalfJung commented Apr 19, 2024

apiraino commented Apr 19, 2024

		@@ -399,7 +401,10 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp {
		if ((f_c.const_.ty().is_signed() \|\| discr_ty.is_signed())

	if ((f_c.const_.ty().is_signed() \|\| discr_ty.is_signed())
	if ((f_c.const_.ty().is_signed() && discr_ty.is_signed())

		@@ -368,6 +368,8 @@ impl<'tcx> SimplifyMatch<'tcx> for SimplifyToExp {
		return None;
		}

Don't perform unsigned comparisons for signed integers #124122

Are you sure you want to change the base?

Don't perform unsigned comparisons for signed integers #124122

Conversation

DianQK commented Apr 18, 2024 • edited

rustbot commented Apr 18, 2024

RalfJung Apr 18, 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

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

RalfJung Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Apr 19, 2024 • edited

DianQK commented Apr 19, 2024

RalfJung commented Apr 19, 2024

apiraino commented Apr 19, 2024

DianQK commented Apr 18, 2024 •

edited

RalfJung Apr 18, 2024 •

edited

RalfJung Apr 18, 2024 •

edited

RalfJung Apr 19, 2024 •

edited

RalfJung Apr 19, 2024 •

edited

RalfJung commented Apr 19, 2024 •

edited