wasmtime / PR #9505 x64: use `ByteSink` more liberally in... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9505 x64: use `ByteSink` more liberally in...

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:32):

abrown requested cfallin for a review on PR #9505.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:32):

abrown requested wasmtime-compiler-reviewers for a review on PR #9505.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:32):

abrown opened PR #9505 from abrown:more-byte-sink to bytecodealliance:main:

In looking at adding auto-generated assembler code to Cranelift, I've noticed that we pass MachBuffer down when it is not always necessary. The ByteSink trait (which MachBuffer implements) is a simplified view to put* bytes into the buffer and it is a bit simpler to use when testing since it is also implemented by Vec<u8>. This change propagates the trait to the rex module.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:35):

abrown commented on PR #9505:

Similar to #9504, this is just an attempt at trying to simplify what is needed to actually assemble an instruction. We still need MachBuffer for instructions that touch memory; thinking about it, it may just be unavoidable.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:36):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:36):

abrown created PR review comment:

Oh, actually let me remove this: this is for the new assembler but isn't used yet.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:37):

abrown updated PR #9505.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:38):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:38):

abrown created PR review comment:

...and a little bit of code movement.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:41):

cfallin submitted PR review:

This looks fine, thanks!

Re: handling Amodes and the helper that briefly appeared here: I think we'll need MachBuffer unavoidably once we have amodes that can refer to labels; that's the semantic thing that MachBuffer gives us above an arbitrary byte sequence. But totally fine to trim the close coupling at lower levels.

I'd love to talk more about the assembler design w.r.t. dependencies -- intuitively I would have thought that MachBuffer would be an integral part of it, i.e. it's the assembler's way of providing its output (given that assemblers need label references, and to flow through other metadata like trap info and relocs from insts to the code blob), but maybe there's another use-case or need for generalization I'm missing.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:41):

cfallin has enabled auto merge for PR #9505.

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:48):

abrown commented on PR #9505:

intuitively I would have thought that MachBuffer would be an integral part of it

I mean, that's how it has to be today. I'm kind of wondering if it should, though: it's nice to be able to "just emit" an instruction into a byte buffer during testing and that's what I've been thinking about. The setup in emit_tests.rs just to be able to emit an instruction is a lot, e.g. If we're proptest-ing or fuzzing we also want the "emit a single instruction" path to be relatively quick.

On the other hand, sometimes its unavoidable: I've notice that we register traps and labels using a MachBuffer in the encoding logic and that simply cannot be done with ByteSink. Maybe ByteSink can gain extra trait functions that do nothing for the Vec<u8> case or something like that...

Wasmtime GitHub notifications bot (Oct 23 2024 at 22:49):

abrown edited a comment on PR #9505:

intuitively I would have thought that MachBuffer would be an integral part of it

I mean, that's how it has to be today. I'm kind of wondering if it should, though: it's nice to be able to "just emit" an instruction into a byte buffer during testing and that's what I've been thinking about. The setup in emit_tests.rs just to be able to emit an instruction is a lot, e.g. If we're proptest-ing or fuzzing we also want the "emit a single instruction" path to be relatively quick.

On the other hand, sometimes it _is_ unavoidable: I've notice that we register traps and labels using a MachBuffer in the encoding logic and that simply cannot be done with ByteSink. Maybe ByteSink can gain extra trait functions that do nothing for the Vec<u8> case or something like that...

Wasmtime GitHub notifications bot (Oct 23 2024 at 23:00):

cfallin commented on PR #9505:

Maybe ByteSink can gain extra trait functions that do nothing for the Vec<u8> case or something like that...

Ah! Yeah, that's an interesting idea: lift the whole interface of MachBuffer into a trait, and provide an implementation of that trait for a Vec<u8>.

There are interesting semantic implementations of this though when label references are involved: what should the assembled bytes from a mov rax, [rip+label] instruction be? Zeroes in the RIP offset (i.e. labels not resolved)? One aspect of the MachBuffer that is nice is that its types essentially force one to be aware one is referencing addresses in the buffer: you have to ask for a label to refer to it, the various amodes require a label, and if you don't bind it, (IIRC?) it should panic when finalizing.

Maybe another way of reading this friction is that we need some convenience APIs? It should be possible to write an assemble(inst: Inst) -> Vec<u8> that is something like:
fn assemble(inst: Inst) -> Vec<u8> {
  let mut buffer = MachBuffer::new();
  inst.emit(&mut inst, ...);
  buffer.finalize(&VCodeConstants::default(), &ControlPlane::default()).apply_base_srcloc(SourceLoc::new(0)).data.into()
}
(whew, that is a bit of a mouthful, I see why you want it!)

Wasmtime GitHub notifications bot (Oct 23 2024 at 23:01):

cfallin edited a comment on PR #9505:

Maybe ByteSink can gain extra trait functions that do nothing for the Vec<u8> case or something like that...

Ah! Yeah, that's an interesting idea: lift the whole interface of MachBuffer into a trait, and provide an implementation of that trait for a Vec<u8>.

There are interesting semantic implementations of this though when label references are involved: what should the assembled bytes from a mov rax, [rip+label] instruction be? Zeroes in the RIP offset (i.e. labels not resolved)? One aspect of the MachBuffer that is nice is that its types essentially force one to be aware one is referencing addresses in the buffer: you have to ask for a label to refer to it, the various amodes require a label, and if you don't bind it, (IIRC?) it should panic when finalizing.

Maybe another way of reading this friction is that we need some convenience APIs? It should be possible to write an assemble(inst: Inst) -> Vec<u8> that is something like:
fn assemble(inst: Inst) -> Vec<u8> {
  let mut buffer = MachBuffer::new();
  inst.emit(&mut buffer, ...);
  buffer.finalize(&VCodeConstants::default(), &ControlPlane::default())
    .apply_base_srcloc(SourceLoc::new(0))
    .data
    .into() // SmallVec into Vec
}
(whew, that is a bit of a mouthful, I see why you want it!)

Wasmtime GitHub notifications bot (Oct 23 2024 at 23:04):

cfallin merged PR #9505.

Last updated: Dec 23 2024 at 12:05 UTC