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

Remove an attempt to reserve a GPR when no GPR clobbering is occurring #2766

Merged
merged 3 commits into from
Jan 26, 2022

Conversation

nagisa
Copy link
Contributor

@nagisa nagisa commented Jan 24, 2022

Description

This PR adds a test for another regression introduced by 8f855cf. What happens here is that the local.get 0 instructions reserve the relevant registers and when it comes a time to codegen a function call, the code fails to move one of the imm64 value to the stack region used to pass arguments.

Turns out we had asserts which became unnecessary as part of an earlier fix submitted in #2723 – we no longer clobber any registers when we move imm64 to a stack, but we still attempted to reserve a temporary register for clobbering. Turns out there are situations where reserving an R9 can fail, and this just happened to be one of them. We don't reserve R9 anywhere else in the codebase, luckily, making scope of this kind of bug fairly limited.

As a future improvement I think it would make sense to see if we can pick_gpr or pick_temp_gpr here and avoid the xchg altogether if there are registers available for use, but I wanted to reduce the scope of this particular PR as much as possible in hopes of landing it more smoothly.

Review

  • Add a short description of the change to the CHANGELOG.md file

@nagisa nagisa requested a review from syrusakbary as a code owner January 24, 2022 18:35
@nagisa nagisa changed the title Unassert Remove an attempt to reserve a GPR when no GPR clobbering is occurring Jan 24, 2022
@matklad
Copy link
Contributor

matklad commented Jan 25, 2022

@syrusakbary
Copy link
Member

It seems tests are failing in master because of tracing-subscriber using a non-safe version of thread-local.

We've just updated Cargo.lock dependencies on master to fix it, so if you could rebase with the latest commits, tests should pass and we should be good to merge.

@syrusakbary syrusakbary merged commit 67be3ea into wasmerio:master Jan 26, 2022
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.

4 participants