Stream: git-wasmtime

Topic: wasmtime / PR #1670 Implement passing arguments by ref fo...

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 15:18):

teapotd opened PR #1670 from win64-pass-by-ref to master:

This PR goes on top of #1510 and fixes legalization of function arguments in win64 ABI - arguments larger than pointer width are passed by reference. Fixes #1651.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 15:56):

bjorn3 created PR Review Comment:

I think this should instead be a new variant of ArgumentLoc.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 15:56):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 16:29):

teapotd submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 16:29):

teapotd created PR Review Comment:

I implemented the change of value into pointer as a new ValueConversion, like splitting or zero-extending, so it's separate from argument location. In case of passing-by-ref, location specifies how pointer (not the original value) should be passed - register or stack. For example legalization of i128 argument for win64 goes as follows:

  1. We start with i128
  2. First pass: i128 is converted to pointer of type i64
  3. Second pass: Now argument has type i64, so we assign register or stack location

As far as I can see the signature legalization just changes argument types without saving information how to convert them and it is later recovered during legalization of calls/entry block params (legalize_abi_value). I introduced legalized_to_pointer flag to be able to detect it has been converted to pointer (and not split, for example).

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 16:50):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 16:50):

bjorn3 created PR Review Comment:

For (i64, i64, i64, i64, i128) is the i128 still passed as a pointer, and then the pointer is stored on the stack like a normal argument that doesn't fit in a register anymore?

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 16:51):

bjorn3 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 17:00):

teapotd submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 07 2020 at 17:00):

teapotd created PR Review Comment:

Yes: https://godbolt.org/z/Ugp_u3

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 18:37):

teapotd updated PR #1670 from win64-pass-by-ref to master:

This PR goes on top of #1510 and fixes legalization of function arguments in win64 ABI - arguments larger than pointer width are passed by reference. Fixes #1651.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 18:59):

teapotd updated PR #1670 from win64-pass-by-ref to master:

This PR goes on top of #1510 and fixes legalization of function arguments in win64 ABI - arguments larger than pointer width are passed by reference. Fixes #1651.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 17:09):

teapotd updated PR #1670 from win64-pass-by-ref to master:

This PR goes on top of #1510 and fixes legalization of function arguments in win64 ABI - arguments larger than pointer width are passed by reference. Fixes #1651.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 17:50):

fitzgen created PR Review Comment:

Nitpick: just to make this a little more clear for readers

    assigning_returns: bool,

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 17:50):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 17:50):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 17:50):

fitzgen created PR Review Comment:

Does this test need all of these things? I would expect

test legalizer
target x86_64

in the filetests/legalizer directory.

It isn't clear to me that we need to go all the way through compilation to exercise the things this test is nominally checking, nor would we need is_pic and opt_level=speed_and_size. Am I missing something?

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 18:04):

teapotd submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 18:04):

teapotd created PR Review Comment:

You're right, I'll change it

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 18:12):

teapotd updated PR #1670 from win64-pass-by-ref to master:

This PR goes on top of #1510 and fixes legalization of function arguments in win64 ABI - arguments larger than pointer width are passed by reference. Fixes #1651.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 18:18):

teapotd requested fitzgen for a review on PR #1670.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:03):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 01 2020 at 18:13):

fitzgen merged PR #1670.

Last updated: Nov 22 2024 at 16:03 UTC