Stream: git-wasmtime

Topic: wasmtime / issue #3589 Release Wasmtime 0.32.0

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 17:49):

github-actions[bot] commented on issue #3589:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:module", "cranelift:wasm", "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 (Dec 08 2021 at 18:10):

fitzgen commented on issue #3589:

I think we should wait on https://github.com/bytecodealliance/wasmtime/pull/3585 getting fixed before doing our next release.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 18:29):

cfallin commented on issue #3589:

+1 to that; uncomfortable releasing with a potential codegen issue.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 19:13):

cfallin commented on issue #3589:

It looks like an issue with iadd_ifcout; the lowering only returns the add result, but it's supposed to be both the add result and the carry flag.

I pushed an update to the assertion to the same branch in my fork (it was asserting just for one dst, not for all, but it's still a mismatch). Will look at the lowering, maybe it's simple to fix, otherwise we can just back out iadd_ifcout from ISLE for now to allow the release, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 19:21):

cfallin commented on issue #3589:

Further interesting twists: iadd_ifcout in the pre-ISLE world also wrote only to its first output (the add result) when lowered by itself. We have a separate pattern-match when iadd_ifcout is used by a trapif to explicltly do the right thing. As I recall, this special-casing is just enough to get the dynamic heap checks to work, and since we eventually want iflags-type values to be replaced by bools and pattern-matching, I don't think we want to fully reify the iflags in ISLE. Probably the best option is to just return a freshly-allocated second temp for the flags for now; anything that uses iflags will not be handled by ISLE so we will not be in danger of actually using it.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 19:36):

cfallin commented on issue #3589:

Err, sorry, just realized I had written the above two comments on the wrong issue; will copy-paste over to the assertion issue and delete here.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 19:37):

cfallin deleted a comment on issue #3589:

It looks like an issue with iadd_ifcout; the lowering only returns the add result, but it's supposed to be both the add result and the carry flag.

I pushed an update to the assertion to the same branch in my fork (it was asserting just for one dst, not for all, but it's still a mismatch). Will look at the lowering, maybe it's simple to fix, otherwise we can just back out iadd_ifcout from ISLE for now to allow the release, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 19:37):

cfallin deleted a comment on issue #3589:

Further interesting twists: iadd_ifcout in the pre-ISLE world also wrote only to its first output (the add result) when lowered by itself. We have a separate pattern-match when iadd_ifcout is used by a trapif to explicltly do the right thing. As I recall, this special-casing is just enough to get the dynamic heap checks to work, and since we eventually want iflags-type values to be replaced by bools and pattern-matching, I don't think we want to fully reify the iflags in ISLE. Probably the best option is to just return a freshly-allocated second temp for the flags for now; anything that uses iflags will not be handled by ISLE so we will not be in danger of actually using it.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 08 2021 at 19:37):

cfallin deleted a comment on issue #3589:

Err, sorry, just realized I had written the above two comments on the wrong issue; will copy-paste over to the assertion issue and delete here.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 13 2021 at 15:36):

alexcrichton commented on issue #3589:

With #3585 landed is this otherwise good-to-go? If so I can try to update the release notes today as well before merging.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 13 2021 at 16:30):

cfallin commented on issue #3589:

I think so, yes!

view this post on Zulip Wasmtime GitHub notifications bot (Dec 13 2021 at 17:38):

fitzgen commented on issue #3589:

Yeah, I think we are good to go now.

Last updated: Oct 23 2024 at 20:03 UTC