Stream: git-wasmtime

Topic: wasmtime / PR #6608 Cranelift: Do not have any callee-sav...

Wasmtime GitHub notifications bot (Jun 20 2023 at 19:52):

fitzgen opened PR #6608 from fitzgen:tail-no-callee-saves to bytecodealliance:main:

That is, mark all allocatable registers as clobbered by tail calls.

Additionally, use (basically) all allocatable registers for passing arguments and returns. ("Basically" because we have to leave at least one register available to hold the callee address for indirect calls or else we'll get regalloc constraint errors.)

I could get three more argument registers on riscv if I refactored compute_arg_locs and/or did something similar to aarch64's compute_arg_locs_tail but it didn't seem worth the hassle.

Wasmtime GitHub notifications bot (Jun 20 2023 at 19:52):

fitzgen requested abrown for a review on PR #6608.

Wasmtime GitHub notifications bot (Jun 20 2023 at 19:52):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #6608.

Wasmtime GitHub notifications bot (Jun 20 2023 at 19:52):

fitzgen requested wasmtime-default-reviewers for a review on PR #6608.

Wasmtime GitHub notifications bot (Jun 20 2023 at 19:52):

fitzgen requested jameysharp for a review on PR #6608.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 20 2023 at 20:00):

fitzgen created PR review comment:

Note: this is just clif-util enabling the trace-log feature by default, not changing cranelift-codegen or any other crate that is actually used as a dep anywhere.

Wasmtime GitHub notifications bot (Jun 21 2023 at 16:59):

jameysharp submitted PR review:

I don't much like the structure of this code; I know you've said you don't either. But this PR doesn't make it worse, so let's go with it.

I spent some time trying to figure out if there was more we could share between the register allocator configuration and these ABI bits, given that all allocatable registers are supposed to be clobbered in this ABI, but that didn't look straightforward.

As future work, I think there's something we can do with RA2's PRegSet to represent all of these different ABI-relevant sets of registers compactly; there's a popcount-like trick for finding the index of the Nth set bit that I think covers most of the uses we need here. (See the "Select the bit position (from the most-significant bit) with the given count (rank)" section of https://graphics.stanford.edu/~seander/bithacks.html.) But that's not necessary to land this.

Wasmtime GitHub notifications bot (Jun 21 2023 at 16:59):

jameysharp submitted PR review:

I don't much like the structure of this code; I know you've said you don't either. But this PR doesn't make it worse, so let's go with it.

I spent some time trying to figure out if there was more we could share between the register allocator configuration and these ABI bits, given that all allocatable registers are supposed to be clobbered in this ABI, but that didn't look straightforward.

As future work, I think there's something we can do with RA2's PRegSet to represent all of these different ABI-relevant sets of registers compactly; there's a popcount-like trick for finding the index of the Nth set bit that I think covers most of the uses we need here. (See the "Select the bit position (from the most-significant bit) with the given count (rank)" section of https://graphics.stanford.edu/~seander/bithacks.html.) But that's not necessary to land this.

Wasmtime GitHub notifications bot (Jun 21 2023 at 16:59):

jameysharp created PR review comment:

Would you add a comment here saying so?

Wasmtime GitHub notifications bot (Jun 21 2023 at 19:42):

fitzgen updated PR #6608.

Wasmtime GitHub notifications bot (Jun 21 2023 at 19:42):

fitzgen has enabled auto merge for PR #6608.

Wasmtime GitHub notifications bot (Jun 21 2023 at 21:06):

fitzgen merged PR #6608.

Last updated: Dec 23 2024 at 12:05 UTC