Wasmtime GitHub notifications bot (Apr 02 2021 at 00:17): Wasmtime GitHub notifications bot (Apr 02 2021 at 00:17):abrown opened PR #2799 from inst-format to main:
In preparation for adding new encoding modes to the x64 backend (e.g. VEX,
EVEX), this change moves all of the current instruction encoding functions to
encodings::rex. This refactor does not change any logic.
Please enter the commit message for your changes. Lines starting<!--
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 (Apr 02 2021 at 00:17):abrown requested bnjbvr for a review on PR #2799.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:17):abrown requested cfallin and bnjbvr for a review on PR #2799.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:18):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:18):abrown created PR Review Comment:
Not sure about what visibility level is best for this module...
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:22):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:22):abrown created PR Review Comment:
In a future change, I would like to:
- factor out the external dependencies to this encoding code (I mean, why not? It would make it a bit easier to test and perhaps this code could be useful elsewhere?)
- implement a few more abstractions like
RexFlagsto make this easier to work with (again, why not? Rust == "zero-cost abstractions," right? :grinning:)Any thoughts for or against this?
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:23):abrown submitted PR Review.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:23):abrown created PR Review Comment:
Eventually other ways to encode instruction formats would get exposed here.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:24):abrown edited PR #2799 from inst-format to main:
In preparation for adding new encoding modes to the x64 backend (e.g. VEX,
EVEX), this change moves all of the current instruction encoding functions to
encodings::rex. This refactor does not change any logic.
<!--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 (Apr 02 2021 at 00:24):abrown updated PR #2799 from inst-format to main.
Wasmtime GitHub notifications bot (Apr 02 2021 at 00:25):abrown requested cfallin, bnjbvr and julian-seward1 for a review on PR #2799.
Wasmtime GitHub notifications bot (Apr 02 2021 at 17:39):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 02 2021 at 17:39):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Apr 02 2021 at 17:39):cfallin created PR Review Comment:
I think crate-public is good for now. We should probably reconsider some of the other
pubsubmodules, to be honest; IMHO we expose too much implementation already; but that's out-of-scope here :-)
Wasmtime GitHub notifications bot (Apr 02 2021 at 17:39):cfallin created PR Review Comment:
+1 to both of these!
In particular to the first point: there is really an x86-64 assembler library begging to escape from this module -- it is "incomplete" in the sense that it only implements the instructions we actually need, but maybe it could be useful for other folks in the Rust world as well, eventually.
Wasmtime GitHub notifications bot (Apr 02 2021 at 18:17):abrown merged PR #2799.
Last updated: Oct 23 2024 at 20:03 UTC