Kmeakin edited PR #9593.
Kmeakin edited PR #9593:
Partially solves https://github.com/bytecodealliance/wasmtime/issues/3573
Kmeakin updated PR #9593.
Kmeakin updated PR #9593.
Kmeakin updated PR #9593.
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>
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.
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?
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?
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.)
Kmeakin submitted PR review.
Kmeakin created PR review comment:
I'm not sure how this slipped in, I don't recall editing it. Anyway, I'll revert it
Kmeakin updated PR #9593.
Kmeakin submitted PR review.
Kmeakin created PR review comment:
The intention is to extend
BuiltinType
to include other types (likeu{8,16,32,64,size},
i{8,16,32,64,size}or
str`) in future PRs
Kmeakin edited PR review comment.
cfallin submitted PR review.
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!
cfallin submitted PR review.
cfallin merged PR #9593.
Last updated: Dec 23 2024 at 12:05 UTC