Stream: git-wasmtime

Topic: wasmtime / PR #10110 asm: introduce a new x64 assembler


view this post on Zulip Wasmtime GitHub notifications bot (Jan 24 2025 at 22:24):

abrown opened PR #10110 from abrown:assembler-upstream to bytecodealliance:main:

This is a first step to providing an external assembler for cranelift-codegen as described in https://github.com/bytecodealliance/wasmtime/issues/41. Each commit has further details, but the summary is that this direction would eventually move all assembler logic out into crates designed for easier checking (e.g., fuzzing).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 24 2025 at 22:43):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 24 2025 at 22:52):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 24 2025 at 22:59):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:13):

abrown updated PR #10110.

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

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:39):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:42):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:42):

abrown created PR review comment:

@cfallin, I'm not a big fan of this KnownOffsetTable approach: it seems like we may want to just add appropriate CodeSink/MachBuffer methods to propagate this kind of thing.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:45):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:45):

abrown created PR review comment:

Looking at this CLIF diffs, I see some potential problems: removed instructions, different immediates, potential and -> or miscompilations... This bears another look.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:46):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:47):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:56):

abrown edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:57):

abrown created PR review comment:

(Pretty sure those add -> or "miscompilations" were just bits changing in the constant pool... not sure why they're changing, though).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 00:57):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 01:06):

abrown requested cfallin for a review on PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 01:06):

abrown has marked PR #10110 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 01:06):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2025 at 01:06):

abrown requested wasmtime-default-reviewers for a review on PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin submitted PR review:

This looks really really good -- thanks for spawning this idea and working to build it out!

Most of my feedback you already got offline as this was being built and the general shape is therefore what I'd expect and am overall quite happy with. A bunch of nits below but nothing major -- let's get this merged and we can iterate and start the migration process.

Thanks again!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Could we add doc-comments here? Most are pretty obvious (name is indeed the name) but our general style is for all pub types to have docs. I guess documenting some of the specifics could be helpful too: does "name" mean what you would write in an assembler, or is it a unique name for a particular variant of an instruction?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

It looks like we're a little inconsistent across our crates but I think we can do:

version.workspace = true
edition.workspace = true
rust-version.workspace = true

to inherit a bit more from the top-level.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

earlier than... Cranelift execution? compiling generated Rust code from the DSL?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Is there a part of the manual we can add a reference to that describes the validity conditions here? (I guess some are implicit e.g. in the description of r and digit implying they're mutually exclusive?)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

slightly confusing error message -- I first read it as "the bits (data content) we are encoding must match some operand's actual value", rather than "bits" in the sense of "width". Maybe "for an immediate, the encoding's expected immediate width must match the declared operand's width"?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Can we have doc-comments on the builder methods? Some of them are fairly obvious (e.g. prefix) but it would be good to have one-sentence reminders on the others (w is the REX.W bit, etc) if only for LSP tooltips/hints as folks write the DSL...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Is this how the Intel manual writes the prefix? (It wouldn't surprise me if so but...) it's a bit confusing in a little-endian world: naively I'd expect 0x66F0 to serialize as F0 66 ... when it's the other way around. If we're not otherwise constrained, can we write it bytewise, e.g. `0x66 0xF0 + "?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

I was surprised not to see 0F when I got here -- so my earlier thought about how two-byte instructions are encoded was wrong. Is the plan to add 0F when we eventually need to encode e.g. 0F 1B, or will we expand the opcode to a u16 or u32 with length?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Could we put this const slice alongside the Feature enum definition so it's easier to update/keep in sync?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

We should note here that "multi-byte opcodes" (e.g. 0F 1B for ud2) are handled by defining 0F to be a prefix and then calling the following byte opcode (not obvious to a casual observer otherwise how this might work).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Are we planning to support encoding 32-bit x86 with this assembler as well eventually? (I think that'd be great if so, not that we need it in the short term) If so, do we need to make any of the descriptions over in Rex more general regarding size prefixes, availability of the REX prefix byte itself, etc? Also are we going to gate register availability (e.g. the top 8 of 16) on features?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Can we add a sentence here describing how this fits into the instruction definitions -- e.g., every instruction has a format, and the format must correspond to the specific instruction encoding's expected operands?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

minor pedantry I guess but does the manual describe it as "default"? My 2025-era x86 CPU still boots up thinking it's an 8086 in 16-bit real mode...

"standard mode" or "expected mode of operation" maybe?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

You've probably thought about this already but just to check: is there any way we could share this code directly? E.g. depend on cranelift-codegen-meta, or factor out another crate (cranelift/meta-utils), or something like that?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

minor thing but could we put these on separate lines?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

What's the reason for the commented-out variant here -- could we add a comment describing why in the code? Or does it still need to be disabled?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Is this for testing? If so can we add a comment saying so? (The specificity of only 2-elem arrays otherwise is somewhat surprising)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Oh no! I think we all prefer Intel syntax and are kind of stuck with AT&T only because of legacy/inertia. It might be worth listing here or somewhere else why we're doing this for now and what the plan is: e.g. will we remove this swap and update all the compile test expectations once we've migrated all instructions to the new DSL?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Egads -- is there a way to configure Capstone to always print in decimal or hex? (Necessary evil otherwise, not your fault, just ugly)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Can we make this a newtype wrapper (pub struct KnownOffset(usize)) instead?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

MinusRsp evokes some sort of subtraction operation in my mind; maybe NonRspGpr or something like that?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

"from is" and ending with "are..." -- incomplete sentence?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Totally fine to do Err(PccError::UnimplementedInst) for now -- I'll need to go and update this once we've moved instructions over that PCC cares about, but PCC isn't fuzzed at the moment anyway until I can make an update/fix pass...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 29 2025 at 18:36):

cfallin created PR review comment:

Perhaps! Were you thinking we'd have specific trait methods for e.g. incoming_arg and slot_offset? Or that we'd still be generic over the set of known offsets but have a query method?

I'm totally fine with the latter, in fact it's probably even cleaner; for the former I'd be concerned about baking details of Cranelift's ABI impl into the lower-level library and would still want to follow the analogy of a real assembler letting one define symbolic constants and use them with specific values plugged in. Happy to look at proposed diff if you have one.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 00:22):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 00:22):

abrown created PR review comment:

I don't think any of the Cranelift crates use version.workspace; I assume that's because they exist within the overall Wasmtime workspace?

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

cfallin submitted PR review.

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

cfallin created PR review comment:

Ah right, I forgot about the Wasmtime-vs-Cranelift-version distinction; you're right, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:02):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:22):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:22):

abrown created PR review comment:

This condition is in section 2.1.2 of the "Instruction Format" chapter of volume 2 of, etc., etc.: "an additional 3-bit opcode field is sometimes encoded..." That chapter may be a good start and I'll link it here but, honestly, there's a lot there and it isn't always easy to pull out all the various conditions. I just started with some pretty standard ones here to catch trivial mistakes early; the real safety is in fuzzing against a known-good disassembler.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:29):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:29):

abrown created PR review comment:

There are no hex hints: e.g., 66 0F 54 /r ANDPD xmm1, xmm2/m128. I'll switch this, retaining the 0x hints.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:31):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:31):

abrown created PR review comment:

Yeah, I just didn't need it at the moment. I would say that it could be completely valid to also expand the opcode width, but let's see what we think when we get to those instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 01:31):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 05:17):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 05:17):

abrown created PR review comment:

Yeah, true 32-bit support requires a bit more work. Since I had received the request to at least consider it, though, I thought that the first and quite crucial step is to set up instructions to be gated on 32-bit support.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 05:24):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 05:24):

abrown created PR review comment:

I agree, we should avoid Cranelift-izing this assembler as much as possible. I guess things I don't like about the known offset table approach is that (a) we rebuild this table for every emissions and (b) building it the way I'm doing it here feels quite fragile. How long before things get out of sync with the keys used during amode conversion?!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 17:48):

abrown created PR review comment:

Leaving it as a type enables us to just tag on the Index<KnownOffset, ...> to the trait and no implementation is needed for vectors and slices. Since I wasn't really in love with this part (see my initial comments above) I took a pretty minimal approach. Let's figure out the big picture for this and then we can decide whether to flesh this out more.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 17:48):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 17:51):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 17:57):

abrown created PR review comment:

I don't think so... it all seems to be hidden away behind a variant in ArchSyntax. I suspect there's more configurability possible with a more x86-specific tool like XED but we're just not ready for that yet. The upside to the fuzzing approach here, though, is that the minute we switch to another disassembler that doesn't have this representation we will see fuzz bugs and be able to remove this code.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 17:57):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:01):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:01):

abrown created PR review comment:

I tried that initially but I see test failures?

FAIL filetests/filetests/pcc/succeed/imul-fuzzbug.clif: compile

Caused by:
    Proof-carrying-code validation error: UnimplementedInst

To reproduce with the Err(...) version: cd cranelift && cargo test --test filetests.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:02):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:02):

abrown created PR review comment:

No and in that test but it's probably used in lowering some other instruction...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:03):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:03):

cfallin created PR review comment:

Oh, I had forgotten we also had that test-suite still active -- OK, no worries, this is fine (and leave the TODO) and I'll get to it in my update pass!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:04):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:04):

cfallin created PR review comment:

Yep, sounds good, it's at least well-tested!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:19):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:33):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 18:33):

abrown created PR review comment:

Well, due to the ISLE bits we need cranelift-codegen-meta to depend on cranelift-assembler-x64-meta, but I guess we could pull the formatting bits into a third crate they both depend on. I think at this point the Formatter and fmtln! logic has diverged a bit since on the assembler side I both simplified and then added interesting bits like the "append the location as a comment" logic.

I'm fine with trying to re-synchronize this formatter code into a third crate though I think I should commit to doing that in a future PR to avoid re-working everything here. But another option is to use a pre-existing crate: a casual glance online suggests quote + prettyplease or even genco could be an alternative. I feel like we're in an in-between place where we don't need the full power of something like quote but now we would be re-inventing parts of it into a new crate...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:52):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:52):

alexcrichton created PR review comment:

Question on this: how does this know that an unsigned value is desired? For example if you have (a: u8) & -2 that means it would fail is_imm8, right? But the x64 instruction should be able to encode that?

With the assembler immediates all annotated with if they're sign/zero-extended should there be Simm8 and Uimm8 and the extractors here would be is_simm8 and is_uimm8?

(also as a side note I find u8::try_from(...) handy in cases of conversion like this so you don't have to do any as casts yourself)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:52):

alexcrichton created PR review comment:

(sorry if this was already covered elsewhere) but how come these are changing from a 32-bit and to an 8-bit and?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:52):

alexcrichton created PR review comment:

This one looks like 3 bytes were shaved off the encoding (the 0x19 => 0x16 in the diff), are the new assembler methods perhaps slightly more optimal in encoding size?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:52):

alexcrichton created PR review comment:

If possible could the Arbitrary methods be left off? And the Arbitrary impl for each instruction have where R::ReadGpr: Arbitrary?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:52):

alexcrichton commented on PR #10110:

(feel free to ignore my comments and respond after landing)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 19:54):

alexcrichton commented on PR #10110:

Oh one blocking thing if that's ok: this adds a hard dependency of cranelift on capstone which I believe wasn't previously there. Would it be possible to move that to a dev-dependency or a feature'd dependency?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:05):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:05):

abrown created PR review comment:

There's a couple of issues that caused us to leave this disabled. There are instructions under the and family that, when a REX prefix is prepended, end up sign-extending their operands. When this encoding is used, they have limitations on their registers: "In 64-bit mode, r/m8 can not be encoded to access the following byte registers if a REX prefix is used: AH, BH, CH, DH." Since we won't be using the high 8-bit slots here, we can probably disregard that, but that was one of the initial concerns. The second concern is that, in documenting these instructions, it is not clear if there are mistakes: some of these REX + ... indicate sign extension for certain instructions but not others (e.g., compare and's RM format and or's RM format. The third is that, with both Capstone and XED, we don't really see a difference in the disassembled string, so our fuzzing cannot check that we correctly emit one instruction and not the other. Currently thinking about what to do...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:05):

alexcrichton commented on PR #10110:

(I sent a PR for that change as https://github.com/abrown/wasmtime/pull/10)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:21):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:21):

abrown created PR review comment:

Yeah, that's what I concluded.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:41):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:41):

abrown created PR review comment:

I think I need to ask some questions internally about this part of the reference manual. I'll add a TODO here with a comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 20:44):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 21:16):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 21:16):

abrown created PR review comment:

I'm a bit mystified: CLIF's icmp should return a truthy Int which reduces down to an 8-bit value here:

https://github.com/bytecodealliance/wasmtime/blob/a0338af84f66cb452fdf4b692d4facb5d052c12d/cranelift/codegen/meta/src/cdsl/typevar.rs#L433-L435

So those band instructions should always have lowered to an andb? Perhaps the original encoding just modified a larger width...

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

abrown submitted PR review.

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

abrown created PR review comment:

Ok, I see what you mean. The assembler doesn't care too much about the sign, it just wants 8 bits, so I chose to use u8 to pass those along. What I was trying to do here (incorrectly) is check that we can fit whatever was in the simm32: i32 into those 8 bits.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 22:49):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 22:49):

alexcrichton created PR review comment:

(I pushed up a commit for this over at https://github.com/abrown/wasmtime/pull/10)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 22:51):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 22:51):

alexcrichton created PR review comment:

I talked a bit more about this with @abrown and the reason for this is that Cranelift today clamps operand sizes to 32-bits in the alu_rmi_r constructor. That means that all 8-bit ands are actually 32-bit ands when emitted to x64. Andrew's PR is (IMO correctly) changing the 8-bit and to use andb instead.

@cfallin do you remember what operand_size_of_type_32_64 was doing there? For example why clamp it to 32-bit? (and if so, are said reasons still applicable today?)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 23:15):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 23:15):

abrown created PR review comment:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 23:45):

abrown updated PR #10110.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 31 2025 at 23:48):

abrown updated PR #10110.

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

abrown updated PR #10110.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

I suspect this is an artifact of the development path: initially we only had the 32- and 64-bit versions of the instruction available, and it's safe to implement 8-bit AND with 32-bit AND, so that's what we did.

I don't know about performance implications -- I do recall vaguely something about "partial-register stalls" if one updates only a part of the destination register but maybe it's fine if the instruction already depends on the register as an input. Assuming that's not a concern (@abrown confirm?) then this seems fine with me.

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

abrown submitted PR review.

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

abrown created PR review comment:

Hm, that _could_ be an issue. Maybe I should just leave the 32-bit versions in place in inst.isle just to be safe. I'll put a note there.

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

abrown updated PR #10110.

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

abrown submitted PR review.

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

abrown created PR review comment:

Ok, see 2c8a276.

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

abrown updated PR #10110.

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

abrown updated PR #10110.

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

abrown commented on PR #10110:

This latest CI failure is under QEMU-emulated s390x. Have we ever observed a segfault here before? I can't imagine this has much to do with the new cranelift-assembler-x64 crate since that is excluded here...

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

abrown edited a comment on PR #10110:

This latest CI failure is under QEMU-emulated s390x. Have we ever observed a segfault here before? I can't imagine this has much to do with the new cranelift-assembler-x64 crate since that is excluded here... I'll try re-running the failed jobs.

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

abrown edited a comment on PR #10110:

This latest CI failure is under QEMU-emulated s390x. Have we ever observed a segfault here before? I can't imagine this has much to do with the new cranelift-assembler-x64 crate since that is excluded in that CI check... I'll try re-running the failed jobs.

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

abrown merged PR #10110.


Last updated: Feb 28 2025 at 02:27 UTC