Stream: git-wasmtime

Topic: wasmtime / PR #10273 asm: sse orpd implementation

view this post on Zulip Wasmtime GitHub notifications bot (Feb 21 2025 at 22:35):

rahulchaphalkar opened PR #10273 from rahulchaphalkar:sse-orpd-manual-rebase to bytecodealliance:main:

This PR adds xmm registers for SSE instructions, and 1 SSE instruction orpd. Includes 2 new locations xmm and rm128 and the relevant logic.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 21 2025 at 22:35):

rahulchaphalkar requested abrown for a review on PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 21 2025 at 22:35):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 21 2025 at 23:44):

github-actions[bot] commented on PR #10273:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown submitted PR review:

This all makes sense to me. I think the main thing here is to fix CI; the minor refactoring I suggest below shouldn't significantly change this, but #10276 may.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown created PR review comment:

Don't you want something like

                    .all(|&op| matches!(op.location.kind(), OperandKind::Imm(_) | OperandKind::FixedReg(_) | OperandKind::XmmReg(_))

?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown created PR review comment:

            xmm | m128 => format!("self.{self}.to_string()"),

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown created PR review comment:

A thought: if we move all of the XMM bits to their own module, xmm.rs, then we don't need this variant here and we can separate the GPR and XMM logic from the pretty-printing below. It's a bit confusing that we ask reg::enc::to_string to print register 0 sometimes as %rax and other times as %xmm0. If we separate this out, then we could refactor to gpr::enc::to_string(enc: u8, size: OperandSize) and xmm::enc::to_string(enc: u8).

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown created PR review comment:

Perhaps add a quote to the SSE section below:

Some SSE2/SSE3/SSSE3/SSE4 instructions and instructions using a three-byte sequence of primary opcode bytes may use 66H as a mandatory prefix to express distinct functionality.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown created PR review comment:

We can just call this Xmm and XmmMem and I'll try to remember to rename Reg to Gpr; or alternatively, we might be able to just fit the XMM versions under OperandKind::Reg and OperandKind::RegMem...

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2025 at 21:54):

abrown created PR review comment:

This generated code looks a lot like the Reg* versions; that adds some points to keeping the XMM bits under the Reg* kinds. we can always use the locations to figure out whether to emit the Gpr* or Xmm* types.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 25 2025 at 20:43):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 25 2025 at 23:22):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 00:02):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:16):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:19):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:19):

rahulchaphalkar created PR review comment:

Changed to check if 128 bits

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:19):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:19):

rahulchaphalkar created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:19):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:19):

rahulchaphalkar created PR review comment:

Folded the xmm bits under reg

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:20):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:20):

rahulchaphalkar created PR review comment:

Folded xmm bits under reg. It does add several match statements in different places to check 128 bits.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:21):

rahulchaphalkar edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:21):

rahulchaphalkar edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:21):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:21):

rahulchaphalkar created PR review comment:

Folded xmm bits under reg.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2025 at 23:22):

rahulchaphalkar commented on PR #10273:

Thanks for the review, @abrown. I've addressed all comments. I'll now look at resolving conflicts.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2025 at 00:29):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 00:58):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 01:05):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 06:21):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 06:40):

rahulchaphalkar commented on PR #10273:

@abrown I've rebased latest main, please take a look.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 17:54):

abrown created PR review comment:

            XmmMem::Xmm(xmm) => enc::to_string(xmm.enc()).to_owned(),

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 17:54):

abrown created PR review comment:

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 17:54):

abrown submitted PR review:

Makes sense to me with a couple small refactorings.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 17:54):

abrown created PR review comment:

We should be able to get rid of this now, right? Perhaps this becomes a panic!("no need to generate a size for XMM-sized access").

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 17:54):

abrown created PR review comment:

            Some(size) => reg::enc::to_string(self.enc(), size),

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:41):

rahulchaphalkar updated PR #10273.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:41):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:41):

rahulchaphalkar created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:41):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:41):

rahulchaphalkar created PR review comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:44):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:44):

rahulchaphalkar created PR review comment:

I need enc from xmm module, similar to let base = reg::enc::to_string(base.enc(), Size::Quadword); needs enc from reg. I could just do use crate::xmm::enc but its easier to distinguish between reg::enc and xmm::enc this way I think.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:45):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 18:45):

rahulchaphalkar created PR review comment:

From above comment, I need to have use crate::xmm

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 19:51):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 19:51):

abrown created PR review comment:

Oh, ok, I thought we were already in xmm.rs here. In that case, why not just use the existing to_string function?

            XmmMem::Xmm(xmm) => xmm.to_string(),

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 22:56):

rahulchaphalkar submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2025 at 22:56):

rahulchaphalkar created PR review comment:

Andrew and I had a chat, and we're going to leave this as is for now. There is some work to be done on to_string front in api.rs in AsReg, where size is an optional argument.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 01 2025 at 07:10):

abrown merged PR #10273.

Last updated: Dec 13 2025 at 19:03 UTC