wasmtime / PR #9287 Make the Tail call conv follow the sy... · git-wasmtime

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "winch"

Thus the following users have been cc'd because of the following labels:

saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:32):

fitzgen submitted PR review:

Thanks! LGTM, although I have some minor nitpicks. If you feel strongly that some of them are unnecessary, I can be convinced otherwise.

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:32):

fitzgen created PR review comment:

            debug_assert_eq!(args_or_rets, ArgsOrRets::Args);

for slightly better panic messages

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:32):

fitzgen created PR review comment:

            assert_eq!(ArgsOrRets::Args, args_or_rets);

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:32):

fitzgen created PR review comment:

Feel similarly here

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:32):

fitzgen created PR review comment:

To make this code a little future proof to shuffling it around at some point in the future, lets not hard code 0:
            next_x_reg += 1;
            Some(ABIArg::reg(
                x_reg(next_x_reg - 1).to_real_reg().unwrap(),
                I64,
                ir::ArgumentExtension::None,
                ir::ArgumentPurpose::Normal,
            ))
It may be convenient to have a helper closure that does the increment of next_x_reg and returns Option<Reg> or whatever for you. I'll leave it up to you, and how much you are annoyed by next_x_reg - 1 :-p

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:36):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:36):

bjorn3 created PR review comment:

There are only two options in this enum and this matches the other debug assertions, but sure.

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:39):

bjorn3 created PR review comment:

This really needs to be the first argument register. If next_x_reg - 1 is not x_start at this point, that is a bug.

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:48):

bjorn3 updated PR #9287.

Wasmtime GitHub notifications bot (Sep 20 2024 at 15:57):

bjorn3 updated PR #9287.

Wasmtime GitHub notifications bot (Sep 20 2024 at 16:02):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 20 2024 at 16:02):

fitzgen has enabled auto merge for PR #9287.

Wasmtime GitHub notifications bot (Sep 20 2024 at 16:26):

fitzgen merged PR #9287.

Last updated: Nov 22 2024 at 16:03 UTC