Stream: git-wasmtime

Topic: wasmtime / PR #7934 Restructure the MacroAssembler interf...

Wasmtime GitHub notifications bot (Feb 14 2024 at 02:00):

elliottt opened PR #7934 from elliottt:trevor/winch-clobbers to bytecodealliance:main:

To prepare for moving the stack check into the function prologue, rework the macro assembler's interface to clobbers to ensure that the unwind info defines the prologue correctly on windows.

<!--
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 (Feb 14 2024 at 03:44):

github-actions[bot] commented on PR #7934:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "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 (Feb 14 2024 at 05:54):

elliottt updated PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 08:09):

elliottt edited PR #7934:

To prepare for moving the stack check into the function prologue, rework the macro assembler's interface to clobbers to ensure that the unwind info defines the prologue correctly on windows. Though clobbers are only used in two trampolines, it's a nice interface to use the save/restore points to bracket the code between the prologue and epilogue.

<!--
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 (Feb 14 2024 at 08:09):

elliottt has marked PR #7934 as ready for review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 08:09):

elliottt requested abrown for a review on PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 08:09):

elliottt requested wasmtime-compiler-reviewers for a review on PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 08:09):

elliottt requested saulecabrera for a review on PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 16:48):

elliottt edited PR #7934:

To prepare for moving the stack check into the function prologue, rework the macro assembler's interface to clobbers to ensure that the unwind info defines the prologue correctly on windows. Though clobbers are only used in two trampolines, it's a nice interface to use the save/restore points to bracket the code between the prologue and epilogue.

I made an additional change that's not strictly necessary in this PR: reserving the space for clobbers with reserve_stack, and then storing values, rather than calling push for each value. I opted to maintain 16-byte stack alignment in this case, despite winch fixing stack alignment up around calls, and this paid off in trampolines that require clobbers as they do one fewer push/pop combination.

<!--
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 (Feb 14 2024 at 17:03):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:03):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:03):

abrown created PR review comment:

Should we have something like:

        debug_assert!(self.callee_saved_regs.is_empty());
        self.masm.restore_clobbers(&[]);

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:11):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:11):

elliottt created PR review comment:

That's a great idea, thanks Andrew!

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:11):

elliottt updated PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:17):

saulecabrera submitted PR review:

Thanks for re-structuring this, and sorry it took me a bit to get to it. I left a couple of small suggestions.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:17):

saulecabrera submitted PR review:

Thanks for re-structuring this, and sorry it took me a bit to get to it. I left a couple of small suggestions.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:17):

saulecabrera created PR review comment:

Would it make sense to use ABI::word_bytes() and ABI::word_bytes() * 2 here?

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:17):

saulecabrera created PR review comment:

Do we still want to keep this debug_assert_eq?

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:20):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:20):

elliottt created PR review comment:

Ah on further thought (and after this assertion failed locally) we unconditionally populate the list of callee save regs, but then the wasm->native trampoline doesn't need to save any registers, as the winch abi does not have any callee saves. We could conditionally populate this based on the the way the Trampoline is compiled, but that feels like we're pushing an invariant out to the caller that would be better enforced through types: maybe the trampoline functions could also call finalize(), and take ownership of self to avoid potentially calling multiple strategies?

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:20):

elliottt updated PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:23):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:23):

elliottt created PR review comment:

I like that! I think it also gives good justification to uncommenting the debug assert you mentioned below, as now it's checking the connection between the word_bytes() and OperandSize::bytes() results.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:23):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:23):

elliottt created PR review comment:

I think so, after the change above. I'll uncomment it :)

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:24):

elliottt updated PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:38):

elliottt updated PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 17:40):

elliottt updated PR #7934.

Wasmtime GitHub notifications bot (Feb 14 2024 at 18:38):

elliottt merged PR #7934.

Last updated: Nov 22 2024 at 17:03 UTC