x64: refactor REX-specific encoding machinery to its own module #2799
Conversation
| @@ -22,6 +22,7 @@ pub mod args; | |||
| mod emit; | |||
| #[cfg(test)] | |||
| mod emit_tests; | |||
| pub(crate) mod encoding; | |||
There was a problem hiding this comment.
Not sure about what visibility level is best for this module...
There was a problem hiding this comment.
I think crate-public is good for now. We should probably reconsider some of the other pub submodules, to be honest; IMHO we expose too much implementation already; but that's out-of-scope here :-)
| }, | ||
| machinst::{MachBuffer, MachInstEmitInfo}, | ||
| }; | ||
| use regalloc::{Reg, RegClass}; |
There was a problem hiding this 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? 😀)
Any thoughts for or against this?
There was a problem hiding this 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.
| @@ -0,0 +1 @@ | |||
| pub mod rex; | |||
There was a problem hiding this comment.
Eventually other ways to encode instruction formats would get exposed here.
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.
github-actions
bot
added
cranelift
| }, | ||
| machinst::{MachBuffer, MachInstEmitInfo}, | ||
| }; | ||
| use regalloc::{Reg, RegClass}; |
There was a problem hiding this 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.
| @@ -22,6 +22,7 @@ pub mod args; | |||
| mod emit; | |||
| #[cfg(test)] | |||
| mod emit_tests; | |||
| pub(crate) mod encoding; | |||
There was a problem hiding this comment.
I think crate-public is good for now. We should probably reconsider some of the other pub submodules, to be honest; IMHO we expose too much implementation already; but that's out-of-scope here :-)
+1 for this. I've tried to use it more generally, because making things generally public with |
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.