Stream: git-wasmtime

Topic: wasmtime / PR #10918 x64: Custom print logic

Wasmtime GitHub notifications bot (Jun 04 2025 at 01:41):

rahulchaphalkar opened PR #10918 from rahulchaphalkar:custom-print to bytecodealliance:main:

This PR implements custom print logic for lock instruction, and reworks the existing logic for custom_visit to be more in the form of .custom(Visit | Display). The custom_visit logic was originally implemented in https://github.com/bytecodealliance/wasmtime/pull/10887 and the changes are made based on some of these comments https://github.com/bytecodealliance/wasmtime/pull/10887#discussion_r2121785272

The Display logic will be used for compare instructions as well.
@abrown

Wasmtime GitHub notifications bot (Jun 04 2025 at 01:41):

rahulchaphalkar requested fitzgen for a review on PR #10918.

Wasmtime GitHub notifications bot (Jun 04 2025 at 01:41):

rahulchaphalkar requested wasmtime-compiler-reviewers for a review on PR #10918.

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:34):

abrown requested abrown for a review on PR #10918.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown submitted PR review:

I think the important bits are mostly here, most of the comments below are smaller Rust issues. The one real issue here is _where_ we put the logic... see below.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

    Encode,

We don't need None here because Custom can be empty.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

pub struct Custom(Vec<CustomOperation>);

No need for this to be pub if you have iter below.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

    /// Indicate this instruction as needing custom processing.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

        if !custom.is_empty() {
            write!(f, " custom({custom})")?;
        }

Since we already have Display implemented for Custom.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

        custom: Custom::default(),

We could #[derive(Default)] to create the empty set or we could create a Custom::empty() -> Self function if we want to be more clear.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

mod custom;

No need to publicize this; we re-publish the types we want to be public in the pub use custom::... below.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

These might not be needed ever, but probably aren't used now (?). Can we remove them?

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

As mentioned above, this should move to cranelift_assembler_x64::custom::display. And, just like visit, we will (1) want to have a function for each instruction that needs the custom display logic and (2) pass through the whole instruction, in case we need more information in the future:

pub mod display {
  pub fn lock_addb(inst: &lock_addb) -> String { ... }
  pub fn lock_addw(inst: &lock_addw) -> String { ... }
  ...
  pub fn lock_subb(inst: &lock_subb) -> String { ... }
  pub fn lock_subw(inst: &lock_subw) -> String { ... }
  ...
}

Two thoughts about this:
- this could be quite repetitive so we might want to use a macro
- eventually we probably want the signature (&<inst>) -> String to be a bit more expansive, i.e., to return the entire display string and not just the altered mnemonic, but this is probably OK for now.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

    /// Whether or not this instruction uses custom, external functions
    /// instead of Rust code generated by this crate.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

mod custom;

?

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

We don't want any implementation of the actual custom functions to live in this crate; we want them to live in the parent crate, cranelift-assembler-x64, in the custom module. So that custom module, which you modified to contain a visit sub-module, should also contain a display sub-module and eventually an encode module, once we need it.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

This should look more like the logic with visit above: if we have a custom display function, then we emit something like:

fmtln!(f, "let inst_name = crate::custom::display::{}(self)", self.name());

And then, in this case, we would use inst_name later in the generated code instead of the already-known inst_name we have here.

Wasmtime GitHub notifications bot (Jun 04 2025 at 16:53):

abrown created PR review comment:

    use crate::{Fixed, Gpr, GprMem, RegisterVisitor, Registers, gpr};

Wasmtime GitHub notifications bot (Jun 05 2025 at 22:08):

rahulchaphalkar updated PR #10918.

Wasmtime GitHub notifications bot (Jun 05 2025 at 22:10):

rahulchaphalkar commented on PR #10918:

Ready for review, addressed feedback

Wasmtime GitHub notifications bot (Jun 05 2025 at 22:41):

abrown submitted PR review:

Thanks! This looks good. I'll put this in the merge queue but if @alexcrichton has any further suggestions we could follow this up in the compare PR.

Wasmtime GitHub notifications bot (Jun 05 2025 at 22:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 05 2025 at 23:20):

abrown merged PR #10918.

Last updated: Dec 06 2025 at 07:03 UTC