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 theAvx512Opcode
,SseOpcode
andAvxOpcode
, 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.
-->
saulecabrera requested cfallin for a review on PR #5413.
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 theAvx512Opcode
,SseOpcode
andAvxOpcode
, 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.
-->
saulecabrera updated PR #5413 from expose-cranelift-codegen-for-winch
to main
.
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 theAvx512Opcode
,SseOpcode
andAvxOpcode
, 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.
-->
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 theAvx512Opcode
,SseOpcode
andAvxOpcode
, 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.
-->
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 theAvx512Opcode
,SseOpcode
andAvxOpcode
, 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.
-->
saulecabrera updated PR #5413 from expose-cranelift-codegen-for-winch
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
s/substraction/subtraction/ (and below)
bjorn3 submitted PR review.
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 usepub(crate)
where things are not meant to be exported?
saulecabrera updated PR #5413 from expose-cranelift-codegen-for-winch
to main
.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Fixed!
saulecabrera submitted PR review.
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
andTextSectionBuilder
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 undermachinst
, to be consistent we should probably not exposeCompiledCode
andTextSectionBuilder
at the top level and move them undermachinst
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.
cfallin submitted PR review.
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.
cfallin merged PR #5413.
Last updated: Dec 23 2024 at 12:05 UTC