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

Don't preemptively spill values. #1641

Closed
wants to merge 1 commit into from

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Mar 6, 2025

We once needed this hack when:

  1. our register allocator was more rubbish than it is today;
  2. we didn't have side traces (or, soon to come, bridge/link traces).

The situation now is a bit different:

a. Preemptively spilling (I think...) doesn't meaningfully help the
register allocator (or, if it does, that's something I should fix). b. For side traces, we end up unspilling everything we spill, even if there were spare registers we could have kept values in.

We once needed this hack when:

  1. our register allocator was more rubbish than it is today;
  2. we didn't have side traces (or, soon to come, bridge/link traces).

The situation now is a bit different:

  a. Preemptively spilling (I think...) doesn't meaningfully help the
     register allocator (or, if it does, that's something I should fix).
  b. For side traces, we end up unspilling everything we spill, even if
     there were spare registers we could have kept values in.
@vext01 vext01 added this pull request to the merge queue Mar 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 6, 2025
@ltratt
Copy link
Contributor Author

ltratt commented Mar 6, 2025

list timed out, but on b16 I only measure a relatively small slowdown, so I wonder if we just got unlucky? Maybe retry?

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

vext01 commented Mar 6, 2025

one more for luck

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

ltratt commented Mar 7, 2025

This probably hits a bigger bug I'm working on. I'm going to close it, because it's probably best tackled as part of that wider effort.

@ltratt ltratt closed this Mar 7, 2025
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.

2 participants