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

Break the optimiser into separate functions. #1558

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Jan 24, 2025

The optimiser has now exceeded even my (high!) tolerance for long functions. Previously it was structured as:

match inst {
  Inst::A => {
    ...
  }
  Inst::B => {
    ...
  }
}

where the ...s could be arbitrarily long.

This commit -- which has no functional change, despite its size! -- changes the structure to:

match inst {
  Inst::A => self.opt_a(...),
  Inst::B => self.opt_b(...),
}

So now each instruction has its "own" function. opt_binop is still fearsomely large, but this commit moves us in a more sustainable direction.

The optimiser has now exceeded even my (high!) tolerance for long
functions. Previously it was structured as:

```rust
match inst {
  Inst::A => {
    ...
  }
  Inst::B => {
    ...
  }
}
```

where the `...`s could be arbitrarily long.

This commit -- which has no functional change, despite its size! --
changes the structure to:

```rust
match inst {
  Inst::A => self.opt_a(...),
  Inst::B => self.opt_b(...),
}
```

So now each instruction has its "own" function. `opt_binop` is still
fearsomely large, but this commit moves us in a more sustainable
direction.
@vext01
Copy link
Contributor

vext01 commented Jan 24, 2025

Bravo!

@vext01 vext01 added this pull request to the merge queue Jan 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 24, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Jan 24, 2025

Bounce failed. Unlikely to be related to this PR! Can we retry please?

@ptersilie ptersilie added this pull request to the merge queue Jan 24, 2025
Merged via the queue into ykjit:master with commit 0295bb7 Jan 24, 2025
2 checks passed
@ltratt ltratt deleted the split_opt branch January 24, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants