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

Fix changes to stackmaps made by the register allocator. #61

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

ptersilie
Copy link

The register allocator may insert spills and spill reloads around function calls. Unfortunately, it doesn't take stackmap instructions into account, so spill reloads may be placed inbetween a call and the stackmap attached to it. This changes the offset of the stackmap and the variables it tracks, which is information we rely upon for deoptimisation.

This adds a new pass which "reverts" these changes, by moving the stackmap instruction back to right below the call, and updates its operand (i.e. tracked variables) as appropriate.

@ltratt
Copy link

ltratt commented Apr 19, 2023

Is the test here as "complete" as we know how to make it? I had the impression from other things that you've come across a few different hard cases, but I wasn't sure if the test in this PR captures all of them or not.

@ptersilie
Copy link
Author

ptersilie commented Apr 19, 2023

Is the test here as "complete" as we know how to make it? I had the impression from other things that you've come across a few different hard cases, but I wasn't sure if the test in this PR captures all of them or not.

Good question. This test was my attempt to recreate cases I saw with lua, to test the old PR. But this test took a different path inside the register allocator that wasn't fixed by the old PR. Which is why this PR has a more general approach to fix stackmaps that doesn't care about the register allocator since it runs after register allocation has happened.

Does this cover all cases? Probably not. Is it the best I can do at the moment until I find stuff that breaks? Probably.

@ltratt
Copy link

ltratt commented Apr 19, 2023

Does this cover all cases? Probably not. Is it the best I can do at the moment until I find stuff that breaks? Probably.

I'm fine adding a couple of other tests that might just be subsets of the one we have. Sometimes, multiple tests can end up catching regressions that just having one wouldn't have.

@ptersilie
Copy link
Author

I'm not quite sure what you're asking. It's difficult to write these kind of tests unless I have seen something fail in an existing code base, since its very hard to predict what sort of machine code LLVM IR input produces. I tried to make a test for the old PR but wasn't able to test the right CFG path, which is why I had to come up with the more general solution in this PR.

@ltratt
Copy link

ltratt commented Apr 20, 2023

What I mean is: shouldn't we also include some of the tests for the "old" PR that it seemed to fix? Those might end up being distinct from the "new" test in this PR?

@ptersilie
Copy link
Author

ptersilie commented Apr 20, 2023

The test here is the test from the old PR. The thing was it didn't test the change from the old PR, because it's hard to write a test in LLVM IR that forces the compiler to execute certain parts of LLVM. This is why this PR has a more general approach, that doesn't rely on certain LLVM parts of the compiler being run. It works even if we use different register allocators, which the previous approach didn't.

@ltratt
Copy link

ltratt commented Apr 20, 2023

OK.

@vext01
Copy link

vext01 commented Apr 20, 2023

LGTM

The register allocator may insert spills and spill reloads around function
calls. Unfortunately, it doesn't take stackmap instructions into account, so
spill reloads may be placed inbetween a call and the stackmap attached to it.
This changes the offset of the stackmap and the variables it tracks, which is
information we rely upon for deoptimisation.

This adds a new pass which "reverts" these changes, by moving the stackmap
instruction back to right below the call, and updates its operand (i.e. tracked
variables) as appropriate.
@ptersilie
Copy link
Author

Squashed.

@ltratt
Copy link

ltratt commented Apr 20, 2023

bors r+

@vext01
Copy link

vext01 commented Apr 20, 2023 via email

@bors
Copy link

bors bot commented Apr 20, 2023

Already running a review

@bors
Copy link

bors bot commented Apr 20, 2023

Build succeeded:

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