Stream: git-wasmtime

Topic: wasmtime / PR #7100 winch(x64): Call indirect

Wasmtime GitHub notifications bot (Sep 28 2023 at 01:29):

saulecabrera opened PR #7100 from saulecabrera:winch-call-indirect to bytecodealliance:main:

This change adds support for the call_indirect instruction to Winch.

Libcalls are a pre-requisite for supporting call_indirect in order to lazily initialy funcrefs. This change adds support for libcalls to Winch by introducing a BuiltinFunctions struct similar to Cranelift's BuiltinFunctionSignatures struct.

In general, libcalls are handled like any other function call, with the only difference that given that not all the information to fulfill the function call might be known up-front, control is given to the caller for finalizing the call.

The introduction of function references also involves dealing with pointer-sized loads and stores, so this change also adds the required functionality to FuncEnv and MacroAssembler to be pointer aware, making it straight forward to derive an OperandSize or WasmType from the target's pointer size.

Finally, given the complexity of the call_indirect instrunction, this change bundles an improvement to the register allocator, allowing it to track the allocatable vs non-allocatable registers, this is done to avoid any mistakes when allocating/de-allocating registers that are not alloctable.

--
prtest:full

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 28 2023 at 01:29):

saulecabrera requested cfallin for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 01:31):

saulecabrera edited PR #7100:

This change adds support for the call_indirect instruction to Winch.

Libcalls are a pre-requisite for supporting call_indirect in order to lazily initialy funcrefs. This change adds support for libcalls to Winch by introducing a BuiltinFunctions struct similar to Cranelift's BuiltinFunctionSignatures struct.

In general, libcalls are handled like any other function call, with the only difference that given that not all the information to fulfill the function call might be known up-front, control is given to the caller for finalizing the call.

The introduction of function references also involves dealing with pointer-sized loads and stores, so this change also adds the required functionality to FuncEnv and MacroAssembler to be pointer aware, making it straight forward to derive an OperandSize or WasmType from the target's pointer size.

Finally, given the complexity of the call_indirect instrunction, this change bundles an improvement to the register allocator, allowing it to track the allocatable vs non-allocatable registers, this is done to avoid any mistakes when allocating/de-allocating registers that are intended to be non-allocatable.

--
prtest:full

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 28 2023 at 10:59):

saulecabrera has marked PR #7100 as ready for review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 10:59):

saulecabrera requested fitzgen for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 10:59):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 10:59):

saulecabrera requested wasmtime-core-reviewers for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 10:59):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 10:59):

saulecabrera requested wasmtime-default-reviewers for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin submitted PR review:

Thanks! A little confusion below for me but the overall shape looks good.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin submitted PR review:

Thanks! A little confusion below for me but the overall shape looks good.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin created PR review comment:

s/siganture/signature/

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin created PR review comment:

It's probably a good idea to future-proof this a bit by using u64s; all ISAs we support today have 32 GPRs or less but this would be a surprising footgun if we ever exceeded that.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin created PR review comment:

s/calcualte/calculate/

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin created PR review comment:

Along with the above comment, let's assert that index is in range here rather than silently truncating (the as u32 cast) and then left-shifting off the end (1 << index) -- perhaps assert!(reg.hw_enc() < MAX_REGS); and define MAX_REGS near where we would use u32 (or u64 as per other comment) for the bitfield above.

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin created PR review comment:

I'm not quite understanding the "unchecked" mechanism here, or the need to defer calculation of stack size needed, etc. -- don't we know the signatures for all our builtins? Could you add a comment describing why this needs to be unchecked?

Wasmtime GitHub notifications bot (Sep 28 2023 at 18:58):

cfallin created PR review comment:

s/Calcualte/Calculate/

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:08):

saulecabrera updated PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:10):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:10):

saulecabrera created PR review comment:

Yeah, good call, I've made this a u64 and introduced a concept of max in the RegBitSet.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:10):

saulecabrera updated PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:12):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:12):

saulecabrera created PR review comment:

I've added more documentation in the function definition, hope this clarifies things.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:12):

saulecabrera requested cfallin for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:19):

saulecabrera updated PR #7100.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:25):

cfallin created PR review comment:

Ah, OK, I understand better now, I think: this is avoiding the special "constructor but also has side-effects on masm" behavior of new above. In that case, I think my feedback is that this feels a bit like a potential footgun, in that it puts the side-effect in an odd, implicit place: I think it'd be better to have a more functional approach here where the constructor allocates a struct of info, and then we have methods on that struct to carry out its effects. So rather than take masm in new (and context), I think it would make sense to explicitly call the register-save method from callsites. It also makes the sequence clearer and more explicit ("save registers, set up args, call") rather than hiding part of that inside what looks like just a constructor.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:39):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2023 at 21:39):

saulecabrera created PR review comment:

Thanks for that suggestion, yeah I agree that this is going to potentially make things clearer, and with this approach we might end up with a more uniform approach to calls. I'll try something along these lines!

Wasmtime GitHub notifications bot (Sep 29 2023 at 12:03):

saulecabrera updated PR #7100.

Wasmtime GitHub notifications bot (Sep 29 2023 at 13:17):

saulecabrera updated PR #7100.

Wasmtime GitHub notifications bot (Sep 29 2023 at 13:42):

saulecabrera requested cfallin for a review on PR #7100.

Wasmtime GitHub notifications bot (Sep 29 2023 at 13:44):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 13:44):

saulecabrera created PR review comment:

I've addressed the your review in https://github.com/bytecodealliance/wasmtime/pull/7100/commits/57cc5dba5f4bf63ec652383b662537cd5868fcf0, your suggestion definitely made things clearer IMO; we can now easily know what's going on at callsites.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:23):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Sep 29 2023 at 16:59):

cfallin submitted PR review:

Looks great, thanks!

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:44):

cfallin merged PR #7100.

Last updated: Dec 23 2024 at 12:05 UTC