Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change SwitchTarget representation in StableMIR #118461

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

celinval
Copy link
Contributor

The new structure encodes its invariant, which reduces the likelihood of having an inconsistent representation. It is also more intuitive and user friendly.

I encapsulated the structure for now in case we decide to change it back.

Notes:

  1. I had to change the Successors type, since there's a conflict on the iterator type. We could potentially implement an iterator here, but I would prefer keeping it simple for now, and add a successors_iter() method if needed.
  2. I removed CoroutineDrop for now since it we never create it. We can add it when we add support to other MIR stages.

@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2023

r? @ouz-a

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

@rustbot 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 Nov 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

@ouz-a
Copy link
Contributor

ouz-a commented Nov 30, 2023

Thanks! This looks good, have you tested if we are correctly printing otherwise ?

@celinval
Copy link
Contributor Author

celinval commented Nov 30, 2023

It looks correct. The following function:

pub fn demux(input: u8) -> u8 {
    match input {
        0 => 10,
        1 => 6,
        2 => 8,
        _ => 0,
    }
}

prints the following:

fn demux(_0: u8) -> u8 {
}
    bb0: {
        switchInt(_1) -> [0: bb1, 1: bb2, 2: bb3, otherwise: bb4]
    }
    bb1: {
        _0 = const 0_u8
        goto -> 5
    }
    bb2: {
        _0 = const 10_u8
        goto -> 5
    }
    bb3: {
        _0 = const 6_u8
        goto -> 5
    }
    bb4: {
        _0 = const 8_u8
        goto -> 5
    }
    bb5: {
        return
    }

I believe the issues with this output are unrelated to this change, and you are addressing them in #118364, right?

@ouz-a
Copy link
Contributor

ouz-a commented Nov 30, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 30, 2023

📌 Commit 9242c5e has been approved by ouz-a

It is now in the queue for this repository.

@bors 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 Nov 30, 2023
@celinval
Copy link
Contributor Author

After inspecting the output, I realized the basic blocks order is incorrect.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2023
The new structure encodes its invariant, which reduces the likelihood
of having an inconsistent representation. It is also more intuitive and
user friendly.

I encapsulated the structure for now in case we decide to change it back.
We currently rely on the order of successors to be conditional branches
first, followed by the otherwise target.
@celinval
Copy link
Contributor Author

The same function:

pub fn demux(input: u8) -> u8 {
    match input {
        0 => 10,
        1 => 6,
        2 => 8,
        _ => 0,
    }
}

now prints:

fn demux(_0: u8) -> u8 {
}
    bb0: {
        switchInt(_1) -> [0: bb2, 1: bb3, 2: bb4, otherwise: bb1]
    }
    bb1: {
        _0 = const 0_u8
        goto -> 5
    }
    bb2: {
        _0 = const 10_u8
        goto -> 5
    }
    bb3: {
        _0 = const 6_u8
        goto -> 5
    }
    bb4: {
        _0 = const 8_u8
        goto -> 5
    }
    bb5: {
        return
    }

@ouz-a
Copy link
Contributor

ouz-a commented Nov 30, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 30, 2023

📌 Commit 9d2c923 has been approved by ouz-a

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 30, 2023
@bors
Copy link
Contributor

bors commented Dec 1, 2023

⌛ Testing commit 9d2c923 with merge c263ccf...

@bors
Copy link
Contributor

bors commented Dec 1, 2023

☀️ Test successful - checks-actions
Approved by: ouz-a
Pushing c263ccf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2023
@bors bors merged commit c263ccf into rust-lang:master Dec 1, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 1, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c263ccf): 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

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

Cycles

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

Binary size

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

Bootstrap: 673.779s -> 673.487s (-0.04%)
Artifact size: 313.43 MiB -> 313.39 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants