Stream: git-wasmtime

Topic: wasmtime / PR #5413 cranelift-codegen: [x64] Prepare cran...

Wasmtime GitHub notifications bot (Dec 09 2022 at 22:33):

saulecabrera opened PR #5413 from expose-cranelift-codegen-for-winch to main:

This commit prepares the x64 pieces from cranelift-codegen to be consumed by Winch for binary emission. This change doesn't introduce or modifies functionality it makes the necessary pieces for binary emission public.

This change also improves documentation where applicable. I decided to introduce #[allow(missing_docs)] for the Avx512Opcode, SseOpcode and AvxOpcode, since no documentation was present there before, and since most of it is documented in ISLE, but happy to add documentation there too.

<!--

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 (Dec 09 2022 at 22:33):

saulecabrera requested cfallin for a review on PR #5413.

Wasmtime GitHub notifications bot (Dec 09 2022 at 22:35):

saulecabrera edited PR #5413 from expose-cranelift-codegen-for-winch to main:

This commit prepares the x64 pieces from cranelift-codegen to be consumed by Winch for binary emission. This change doesn't introduce or modifies functionality it makes the necessary pieces for binary emission public.

This change also improves documentation where applicable. I decided to introduce #[allow(missing_docs)] for the Avx512Opcode, SseOpcode and AvxOpcode, since no documentation was present there before, but happy to add documentation there too.

<!--

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 (Dec 09 2022 at 22:37):

saulecabrera updated PR #5413 from expose-cranelift-codegen-for-winch to main.

Wasmtime GitHub notifications bot (Dec 09 2022 at 22:42):

saulecabrera edited PR #5413 from expose-cranelift-codegen-for-winch to main:

This commit prepares the x64 pieces from cranelift-codegen to be consumed by Winch for binary emission. This change doesn't introduce or modifies functionality it makes the necessary pieces for binary emission public.

This change also improves documentation where applicable. I decided to introduce #[allow(missing_docs)] for the Avx512Opcode, SseOpcode and AvxOpcode, since no documentation was present there before, but happy to add documentation there too.

I have a binary emission proof-of-concept branch that I'm using a base to extract individual PRs.

<!--

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 (Dec 09 2022 at 22:50):

saulecabrera edited PR #5413 from expose-cranelift-codegen-for-winch to main:

This commit prepares the x64 pieces from cranelift-codegen to be consumed by Winch for binary emission. This change doesn't introduce or modifies functionality it makes the necessary pieces for binary emission public.

This change also improves documentation where applicable. I decided to introduce #[allow(missing_docs)] for the Avx512Opcode, SseOpcode and AvxOpcode, since no documentation was present there before, but happy to add documentation there too if we want to document those opcodes.

I have a binary emission proof-of-concept branch that I'm using a base to extract individual PRs.

<!--

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 (Dec 09 2022 at 23:03):

saulecabrera edited PR #5413 from expose-cranelift-codegen-for-winch to main:

This commit prepares the x64 pieces from cranelift-codegen to be consumed by Winch for binary emission. This change doesn't introduce or modifies functionality it makes the necessary pieces for binary emission public.

This change also improves documentation where applicable. I decided to introduce #[allow(missing_docs)] for the Avx512Opcode, SseOpcode and AvxOpcode, but happy to add documentation there too if we want to document those opcodes.

I have a binary emission proof-of-concept branch that I'm using a base to extract individual PRs.

<!--

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 (Dec 09 2022 at 23:24):

saulecabrera updated PR #5413 from expose-cranelift-codegen-for-winch to main.

Wasmtime GitHub notifications bot (Dec 10 2022 at 00:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2022 at 00:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2022 at 00:17):

cfallin created PR review comment:

s/substraction/subtraction/ (and below)

Wasmtime GitHub notifications bot (Dec 10 2022 at 13:56):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2022 at 13:56):

bjorn3 created PR review comment:

I don't think all of these should be exported at the top level. Most people won't use them, so it may be confusing. Would it make sense to make machinst public and instead use pub(crate) where things are not meant to be exported?

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

saulecabrera updated PR #5413 from expose-cranelift-codegen-for-winch to main.

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

saulecabrera submitted PR review.

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

saulecabrera created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Dec 12 2022 at 12:51):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Dec 12 2022 at 12:51):

saulecabrera created PR review comment:

I had considered this. But didn't do it mainly for two reasons: (i) there's already a precedent: we expose CompiledCode and TextSectionBuilder at the top level; so even though there are no other uses cases aside from Winch that will use the other primitives, personally I feel it makes sense to expose the core binary emission concepts at the top level. (ii) If we want to expose them under machinst, to be consistent we should probably not expose CompiledCode and TextSectionBuilder at the top level and move them under machinst instead. But this will be a breaking change for anyone relying on those exports.

Since Winch is the only use-case relying on this, I'm happy to change it if/once we have a concrete example of an issue caused by this approach.

Wasmtime GitHub notifications bot (Dec 12 2022 at 17:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 12 2022 at 17:00):

cfallin created PR review comment:

FWIW, I think the current approach is fine, too: I agree with @saulecabrera that we'd need more refactoring to be consistent with a separation of exports into modules.

Wasmtime GitHub notifications bot (Dec 12 2022 at 17:01):

cfallin merged PR #5413.

Last updated: Oct 23 2024 at 20:03 UTC