Stream: git-wasmtime

Topic: wasmtime / PR #5319 Remove sig data arg and ret fields to...

Wasmtime GitHub notifications bot (Nov 23 2022 at 08:09):

tnachen opened PR #5319 from tnachen/sigdata_size to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:37):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:37):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:37):

jameysharp created PR review comment:

This isn't important, but I personally prefer to not use a closure if it's just passing its argument to a function or a tuple constructor. Like this:

        self.0.checked_sub(1).map(Sig)

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:37):

jameysharp created PR review comment:

This version of num_args would be slightly simpler if it looked more like the old definition of Caller::num_args:

        let len = self.args(sig).len();
        if self.sigs[sig].stack_ret_arg.is_some() {
            len - 1
        } else {
            len
        }

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:37):

jameysharp created PR review comment:

The result of calling slice::len is a usize, so this call to usize::from isn't necessary in this version.

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:43):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:43):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 17:58):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 25 2022 at 06:56):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 25 2022 at 07:12):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 25 2022 at 07:16):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 25 2022 at 17:50):

tnachen edited PR #5319 from tnachen/sigdata_size to main:

Addressing #5183
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:22):

cfallin created PR review comment:

Pre-existing, but since Sig is now a newtype wrapper around u32 and is Copy, we should probably just take an arg abi: Sig and avoid this dereference.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:22):

cfallin created PR review comment:

Can we add a note here on the indexing scheme? Something like "Arguments for a given sig are in the args array at indicies args_end[sig - 1]..args_end[sig], or 0..args_end[sig] if sig is 0.

(And the args_end value is an exclusive endpoint, i.e. not included, so .. above is correct, right?)

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:22):

cfallin created PR review comment:

Or, reading more below, I guess it's a little more complex because args and rets are contiguous for a given Sig, but we should give the appropriate ranges here and describe the scheme regardless.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:23):

cfallin edited PR review comment.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:32):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 28 2022 at 19:32):

jameysharp created PR review comment:

I had @tnachen make some changes along these lines in a previous PR, but I decided not to ask for that in function signatures that weren't otherwise changed in this PR.

It would be a good follow-up PR to fix all the functions that currently take &Sig to take Sig instead. I currently see examples in cranelift/codegen/src/isa/s390x/lower/isle.rs, cranelift/codegen/src/machinst/abi.rs, and cranelift/codegen/src/machinst/isle.rs.

Wasmtime GitHub notifications bot (Nov 30 2022 at 07:25):

tnachen updated PR #5319 from tnachen/sigdata_size to main.

Wasmtime GitHub notifications bot (Nov 30 2022 at 07:30):

tnachen created PR review comment:

@cfallin added more comments, let me know if this looks good?

Wasmtime GitHub notifications bot (Nov 30 2022 at 07:30):

tnachen submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 07:30):

tnachen submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 07:30):

tnachen created PR review comment:

Sg will make these changes after this one merged

Wasmtime GitHub notifications bot (Nov 30 2022 at 15:18):

cfallin created PR review comment:

Looks great, thanks!

Wasmtime GitHub notifications bot (Nov 30 2022 at 15:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 15:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 15:19):

cfallin merged PR #5319.

Wasmtime GitHub notifications bot (Dec 03 2022 at 03:57):

tnachen submitted PR review.

Wasmtime GitHub notifications bot (Dec 03 2022 at 03:57):

tnachen created PR review comment:

I try to make this change but looks like &Sig is generated from isle as generated code, and all non-primitive types declared as arg funcs in isle seems to be all generated as references. Should we leave this or try to change how isle code gen works?

Wasmtime GitHub notifications bot (Dec 03 2022 at 04:00):

tnachen submitted PR review.

Wasmtime GitHub notifications bot (Dec 03 2022 at 04:00):

tnachen created PR review comment:

Or change (type Sig extern (enum)) into (type Sig (primitive u32)) ?

Wasmtime GitHub notifications bot (Dec 03 2022 at 04:01):

tnachen submitted PR review.

Wasmtime GitHub notifications bot (Dec 03 2022 at 04:01):

tnachen created PR review comment:

Actually that doesn't work as it only generate args with u32 and not Sig anymore

Wasmtime GitHub notifications bot (Dec 06 2022 at 19:47):

jameysharp created PR review comment:

Oh, I see! I just learned how ISLE decides when to borrow last week while I've been rewriting its code generation, so this hadn't occurred to me.

It isn't important to get rid of the &Sig borrows. But if you'd like to try working on the ISLE compiler, I think I can now guide you through fixing this. Let me know if you're interested; if so, I'll write up a plan.

Wasmtime GitHub notifications bot (Dec 06 2022 at 19:47):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2022 at 07:41):

tnachen submitted PR review.

Wasmtime GitHub notifications bot (Dec 07 2022 at 07:41):

tnachen created PR review comment:

@jameysharp I'm game! I think I kind of have a idea looking at a codebase, but I'm still new :)

Wasmtime GitHub notifications bot (Dec 11 2022 at 23:58):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 11 2022 at 23:58):

jameysharp created PR review comment:

So I looked into this more, and I think this doesn't need ISLE compiler changes. I think it should work to change (type Sig extern (enum)) to (type Sig (primitive Sig)) and then fix the hand-written parts to match. Do you want to give that a try?

Last updated: Dec 23 2024 at 12:05 UTC