Stream: git-wasmtime

Topic: wasmtime / PR #9593 ISLE: Add proper bool type

Wasmtime GitHub notifications bot (Nov 11 2024 at 19:16):

Kmeakin edited PR #9593.

Wasmtime GitHub notifications bot (Nov 11 2024 at 19:17):

Kmeakin edited PR #9593:

Partially solves https://github.com/bytecodealliance/wasmtime/issues/3573

Wasmtime GitHub notifications bot (Nov 11 2024 at 20:06):

Kmeakin updated PR #9593.

Wasmtime GitHub notifications bot (Nov 11 2024 at 20:14):

Kmeakin updated PR #9593.

Wasmtime GitHub notifications bot (Nov 11 2024 at 20:23):

Kmeakin updated PR #9593.

Wasmtime GitHub notifications bot (Nov 11 2024 at 22:44):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

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

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

• cfallin: isle
• fitzgen: isle

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Nov 12 2024 at 00:03):

cfallin submitted PR review:

Thanks for working on this -- it's nice to see motion on the "nice-to-have" upgrades we've had in our backlog for a while!

Mostly looks fine; my main thought is about the BuiltinType abstraction and whether we can separate the change out.

Wasmtime GitHub notifications bot (Nov 12 2024 at 00:03):

cfallin created PR review comment:

I'm not sure I understand the reason for having a notion of "built-in types" for bool but not for other types that we have constants for (namely integers) -- while on the other hand it adds some complication to the IR. Maybe we could separate out this part into another PR and discuss separately?

Wasmtime GitHub notifications bot (Nov 12 2024 at 00:03):

cfallin created PR review comment:

I don't see any non-whitespace diffs in this file (I may have missed some?) -- any reason it's included in the PR?

Wasmtime GitHub notifications bot (Nov 12 2024 at 00:03):

cfallin created PR review comment:

(To make it more explicit: I think we probably should have this eventually; but for integers too, and then we should check that integer literals are used only in contexts that expect them.)

Wasmtime GitHub notifications bot (Nov 12 2024 at 14:13):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Nov 12 2024 at 14:14):

Kmeakin created PR review comment:

I'm not sure how this slipped in, I don't recall editing it. Anyway, I'll revert it

Wasmtime GitHub notifications bot (Nov 12 2024 at 14:19):

Kmeakin updated PR #9593.

Wasmtime GitHub notifications bot (Nov 12 2024 at 14:21):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Nov 12 2024 at 14:21):

Kmeakin created PR review comment:

The intention is to extend BuiltinType to include other types (like u{8,16,32,64,size}, i{8,16,32,64,size} or str`) in future PRs

Wasmtime GitHub notifications bot (Nov 12 2024 at 14:21):

Kmeakin edited PR review comment.

Wasmtime GitHub notifications bot (Nov 14 2024 at 22:55):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 14 2024 at 22:55):

cfallin created PR review comment:

OK, yeah, on further thought I suppose it's totally fine to migrate integers over as a second step -- as long as the intention is there to finish out the refactor. Thanks!

Wasmtime GitHub notifications bot (Nov 14 2024 at 22:55):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 14 2024 at 23:11):

cfallin merged PR #9593.

Last updated: Nov 22 2024 at 17:03 UTC