Stream: git-wasmtime

Topic: wasmtime / PR #10066 Winch: Even tighter Extends

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

MarinPostma requested wasmtime-compiler-reviewers for a review on PR #10066.

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

MarinPostma requested fitzgen for a review on PR #10066.

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

MarinPostma opened PR #10066 from MarinPostma:movzv-unsigned-extend to bytecodealliance:main:

Followup to https://github.com/bytecodealliance/wasmtime/pull/10053

This PR has two commit:

1) The first commit makes mov* instruct receive the adequate Extend operation. And it makes extend conversion operation harder to misuse.
2) The second commit it optional. This is an alternative to Signed/UnsignedExtend, that merges the two into a Extend<T>, and factorizes common behaviour, avoiding repetition. The T is either Signed or Zero, with precautions in the design so that it's hard to misuse.

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

MarinPostma edited PR #10066:

Followup to https://github.com/bytecodealliance/wasmtime/pull/10053

This PR has two commit:

1) The first commit makes mov* instruct receive the adequate Extend operation. And it makes extend conversion operation harder to misuse.
2) The second commit is optional. This is an alternative to Signed/UnsignedExtend, that merges the two into a Extend<T>, and factorizes common behaviour, avoiding repetition. The T is either Signed or Zero, with precautions in the design so that it's hard to misuse.

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

saulecabrera submitted PR review:

Thanks for this. Left two minor comments inline.

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

saulecabrera created PR review comment:

To avoid an unused member, could you use PhantomData to hold the type instead? https://doc.rust-lang.org/std/marker/struct.PhantomData.html

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

saulecabrera created PR review comment:

Small naming convention/suggestion: we usually use src, dst for explicit moves, could we keep from_bits and to_bits? Although given that this now returns OperandSize it could maybe be from_size and to_size?

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

MarinPostma submitted PR review.

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

MarinPostma created PR review comment:

do you mean for every variant? like I64ExtendI8(PhantomData<T>)? I think that would make construction pretty cumbersome, no?
If you mean __Kind(PhamtomData<T>), then that defeats the purpose of this variant being unconstructible, since you can't construct Signed and Zero

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

MarinPostma created PR review comment:

No problem. It's just that it kinda overlaps with usual rust naming convention for from_ and to_, which I found a bit confusing. I'll make the change now.

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

MarinPostma submitted PR review.

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

MarinPostma updated PR #10066.

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

saulecabrera submitted PR review.

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

saulecabrera created PR review comment:

I meant more around the lines of:

pub enum ExtendBits {
    /// 8 to 32 bit extend.
    I32Extend8,
    /// 16 to 32 bit extend.
    I32Extend16,
    /// 8 to 64 bit extend.
    I64Extend8,
    /// 16 to 64 bit extend.
    I64Extend16,
    /// 32 to 64 bit extend.
    I64Extend32,
}

pub struct Extend<T> {
    bits: ExtendBits,
    marker: PhantomData<T>,
}

Which is the kind of job that PhantomData is for.

That way you can avoid the unused enum member. In your approach Signed and Zero are mostly used as type flags to ensure that the extension is associated correctly, but even though your comment states that the __Kind(T) variant cannot be constructed, from a practical perspective, you could do Extend::__Kind(Signed) I believe.

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

saulecabrera submitted PR review.

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

saulecabrera created PR review comment:

The general disadvantage that I see is that this approach adds a bit more complexity to the implementation.

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

saulecabrera submitted PR review.

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

saulecabrera created PR review comment:

But we can always iterate on this after landing this change, I think regardless of the unused member, extends are in a better place now.

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

saulecabrera submitted PR review.

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

MarinPostma submitted PR review.

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

MarinPostma created PR review comment:

| from a practical perspective, you could do Extend::__Kind(Signed) I believe.

that's in fact not possible :) You would need to exhibit a variant, but there are none, c.f: https://doc.rust-lang.org/reference/items/enumerations.html#zero-variant-enums

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

MarinPostma submitted PR review.

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

MarinPostma created PR review comment:

I think your proposal could work nicely too, if we implemented Deref<Target = ExtendBits> for Extend<T: ExtendType>

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

MarinPostma submitted PR review.

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

MarinPostma created PR review comment:

we'd need to add contructors to make instanciation simpler, though, which may hurt readability

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

saulecabrera submitted PR review.

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

saulecabrera created PR review comment:

Oh I see, I overlooked, I read this as struct not enum (for Signed and Zero), but this makes sense. Thanks for clarifying.

we'd need to add contructors to make instanciation simpler, though, which may hurt readability

Yeah, I think the current approach is simple enough, if there's a strong reason to change it in the future then we can. For now I'd suggest we keep it as is.

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

saulecabrera merged PR #10066.

Last updated: Jan 24 2025 at 00:11 UTC