Stream: git-wasmtime

Topic: wasmtime / PR #9610 ISLE: built-in integer types

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 01:50):

Kmeakin opened PR #9610 from Kmeakin:km/isle/int-types to bytecodealliance:main:

Follow up to https://github.com/bytecodealliance/wasmtime/pull/9593, adding all the built-in integer types (u8 .. u128, i8 .. i128).

Integer literal expressions and patterns are still allowed to type-check against primitive types, because there are a lot of places in the lowering/optimization code that uses (type Foo (primitive Foo)) where Foo is declared as a type alias of an integer type in Rust code. Not sure what to do about this. Perhaps it would be clearer to rename "primitive" types to "opaque" or "extern" types (and also have a mechanism for declaring transparent type aliases in ISLE)?

Closes https://github.com/bytecodealliance/wasmtime/issues/5431

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 01:50):

Kmeakin requested abrown for a review on PR #9610.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 01:50):

Kmeakin requested wasmtime-compiler-reviewers for a review on PR #9610.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 01:51):

Kmeakin edited PR #9610:

Follow up to https://github.com/bytecodealliance/wasmtime/pull/9593, adding all the built-in integer types (u8 .. u128, i8 .. i128 plus usize and isize).

Integer literal expressions and patterns are still allowed to type-check against primitive types, because there are a lot of places in the lowering/optimization code that uses (type Foo (primitive Foo)) where Foo is declared as a type alias of an integer type in Rust code. Not sure what to do about this. Perhaps it would be clearer to rename "primitive" types to "opaque" or "extern" types (and also have a mechanism for declaring transparent type aliases in ISLE)?

Closes https://github.com/bytecodealliance/wasmtime/issues/5431

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 01:54):

Kmeakin edited PR #9610:

Follow up to https://github.com/bytecodealliance/wasmtime/pull/9593, adding all the built-in integer types (u8 .. u128, i8 .. i128 plus usize and isize).

Integer literal expressions and patterns are still allowed to type-check against primitive types, because there are a lot of places in the lowering/optimization code that uses (type Foo (primitive Foo)) where Foo is declared as a type alias of an integer type in Rust code. Not sure what to do about this. Perhaps it would be clearer to rename "primitive" types to "opaque" or "extern" types (and also have a mechanism for declaring transparent type aliases in ISLE)?

Closes https://github.com/bytecodealliance/wasmtime/issues/5431 and https://github.com/bytecodealliance/wasmtime/issues/3573

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 02:05):

Kmeakin updated PR #9610.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 03:01):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "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 (Nov 15 2024 at 07:52):

cfallin submitted PR review:

Thanks! Some comments below but nothing major. This is a nice refactor and makes the language semantically a little cleaner; I like it.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 07:52):

cfallin created PR review comment:

I'm not sure I like this rename -- semantically I think of interning as the action of getting the existing ID, or creating a new one -- that is, "to intern" covers both cases; and intern_mut indicates we're permitting new cases to update the table, vs. intern that only looks up existing cases.

(To see this another way, consider intern below -- it should be renamed to something else if we take this rename, because this rename implies that adding to the table is the "intern" bit, which is exactly the bit that the non-mut version below does not do.)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 15 2024 at 07:52):

cfallin created PR review comment:

Can we write a match with explicit arms for all types here? I'd prefer that so we are forced to think about any new types we add explicitly. The pattern as written makes that easy to miss...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2024 at 19:27):

Kmeakin updated PR #9610.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 19:57):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 19 2024 at 19:58):

cfallin commented on PR #9610:

Something is broken in the build in CI; once it's green I'm happy to merge.

Last updated: Nov 22 2024 at 16:03 UTC