Stream: git-wasmtime

Topic: wasmtime / issue #5307 Reduce SigData size further

Wasmtime GitHub notifications bot (Nov 21 2022 at 07:08):

tnachen commented on issue #5307:

@jameysharp changing the Option<u16> to bool made the SigData size from 40 -> 32.
However, I'm trying to figure out how to remove more fields based on the SigSet suggestion.
Based on your earlier comment, seems like the motivation is to be able to index over SigDatas since they're all lined up in the abi args list.
But from the function calls it seems like we're parsing a function sig one at a time and not sure what the change should look like here.

Wasmtime GitHub notifications bot (Nov 21 2022 at 17:06):

bjorn3 commented on issue #5307:

I'll defer to @fitzgen and @cfallin on whether it's okay to make stack_ret_arg a bool. You've shown that every ABI we currently support places that argument at the end, which I think is a pretty good argument for doing this. But we want to support more targets in the future, so maybe we don't want to over-simplify this detail? On the other hand, we can always change this again if we ever find a target that doesn't put the stack ret arg at the end.

The position shouldn't matter I think. Only the register assignment.

Wasmtime GitHub notifications bot (Nov 21 2022 at 20:32):

jameysharp commented on issue #5307:

I talked with @fitzgen and @cfallin about this offline. We aren't comfortable hard-coding the assumption that stack_return_arg is always last right now. (@bjorn3, you're right that the register assignment is all that matters, but as I understand it the register assignment gets computed from this list, so order still matters.)

So @tnachen, could you remove the stack_return_arg changes from this PR? I want to merge the rest of the work you did here. Let me know whether you want to try de-duplicating the args_start fields in this PR, or if I should merge this and you can open another PR. I would recommend opening another PR for that unless you feel you can do it within a day or two, just so we can get the work you've already done merged promptly.

Wasmtime GitHub notifications bot (Nov 22 2022 at 00:04):

tnachen commented on issue #5307:

@jameysharp ah ok! I just reverted those changes and kept the Sigdata refactoring.
We can merge this as-is and I can submit another one for the return field changes

Wasmtime GitHub notifications bot (Nov 22 2022 at 03:03):

tnachen commented on issue #5307:

@jameysharp addressed comments!

Last updated: Nov 22 2024 at 16:03 UTC