Fix possibly missed optimization.

[ilyalesokhin-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[orizi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @liorgold2)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

            candidate.match_variable = var_usage.var_id;
            handled_remmapings = 1;
        };

Suggestion:

        if remapping.is_empty() { <----- Doesn't this already do that?
            // Do nothing. Keep the candidate if exists.
            return;
        }

        info.demand.apply_remapping(
            &mut EmptyDemandReporter {},
            remapping.iter().map(|(dst, src)| (dst, (&src.var_id, ()))),
        );

        let Some(ref mut candidate) = &mut info.candidate else {
            return;
        };

        let orig_match_variable = candidate.match_variable;
        let mut handled_remmapings = 0;
        if let Some(var_usage) = remapping.get(&candidate.match_variable) {
            candidate.match_variable = var_usage.var_id;
            handled_remmapings = 1;
        };

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

            candidate.match_variable = var_usage.var_id;
            handled_remmapings = 1;
        };

No, the fix is for the case where remapping.len() ==1, but the match variable is not remapped.

[orizi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @liorgold2)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

No, the fix is for the case where remapping.len() ==1, but the match variable is not remapped.

but this is exactly the deleted line:

        let Some(var_usage) = remapping.get(&candidate.match_variable) else {
            // Revoke the candidate.
            info.candidate = None;
            return;
        };

isn't it?

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

Previously, orizi wrote…

but this is exactly the deleted line:

        let Some(var_usage) = remapping.get(&candidate.match_variable) else {
            // Revoke the candidate.
            info.candidate = None;
            return;
        };

isn't it?

Before the fix, the candidate is always revoked in that case.
after the fix it is not revoked unless there are other issues.

[orizi]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @liorgold2)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Before the fix, the candidate is always revoked in that case.
after the fix it is not revoked unless there are other issues.

can you think of an example this actually happens?

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 334 at r2 (raw file):

        if remapping.len() > handled_remappings {
            // here, we have remappings for variables other than the match variable.

Suggestion:

        let mut has_unhandled_remappings = true;
        if let Some(var_usage) = remapping.get(&candidate.match_variable) {
            candidate.match_variable = var_usage.var_id;
            has_unhandled_remappings = remappings.len() > 1;
        };

        if has_unhandled_remappings {
            // here, we have remappings for variables other than the match variable.

[orizi]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @liorgold2)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 333 at r2 (raw file):

        };

        if remapping.len() > handled_remappings {

Suggestion:

        let has_handled_remappings = 
        if let Some(var_usage) = remapping.get(&candidate.match_variable) {
            candidate.match_variable = var_usage.var_id;
            remappings.len() > 1
        } else { true };

        if has_handled_remappings {

[ilyalesokhin-starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @orizi)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

Previously, orizi wrote…

can you think of an example this actually happens?

no

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 333 at r2 (raw file):

        };

        if remapping.len() > handled_remappings {

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @liorgold2)

---

a discussion (no related file):
note you probably need to rebase and update your rust formatter.

---

crates/cairo-lang-lowering/src/optimizations/test_data/match_optimization line 1163 at r3 (raw file):

//! > ==========================================================================

//! > Remapping where input var is not renamed.

what is the test case?

Code quote:

//! > Remapping where input var is not renamed.

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/optimizations/test_data/match_optimization line 1163 at r3 (raw file):

Previously, orizi wrote…

what is the test case?

I tried to reproduce the issue but couldn't

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

no

I guess it can't happen.

but the origianl code still fills strange to me.

[orizi]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @liorgold2)

---

crates/cairo-lang-lowering/src/optimizations/test_data/match_optimization line 1177 at r3 (raw file):

    if v {
        a = 2;
    }

maybe?
so you'd have a longer merge with possibly more vars?

Suggestion:

    if v {
        a = 1;
        if v {
            a = 2;
        }
    }
    let v = true;
    if v {
        a = 2;
    }

[orizi]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @liorgold2)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 330 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I guess it can't happen.

but the origianl code still fills strange to me.

i can't conceptualize it right now - so i'm really not sure.

[ilyalesokhin-starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @liorgold2 and @orizi)

---

crates/cairo-lang-lowering/src/optimizations/test_data/match_optimization line 1177 at r3 (raw file):

Previously, orizi wrote…

maybe?
so you'd have a longer merge with possibly more vars?

the merge will always happen before the assignment to v, so I can't trillger the issue.

[orizi]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)