Stream: git-wasmtime

Topic: wasmtime / PR #8850 riscv64: Update `Inst::worst_case_size`

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 19:05):

afonso360 opened PR #8850 from afonso360:issue8847 to bytecodealliance:main:

:wave: Hey,

This PR updates the Inst::worst_case_size size for the RISC-V backend. The panic that happens in #8847 is entirely due to the return_call_indirect generating too many bytes.

I found it difficult to add an automatic calculation of the worst possible size for that instruction to the test that we have, so I attempted to manually calculate the worst case size and used that.

The two test cases here are the original test case, and a minimized version without zicond. I'm not entirely sure why it bisects to the ZiCond PR (https://github.com/bytecodealliance/wasmtime/pull/8695), but having both test cases is not too much of a burden, so might as well.

The increased worst case size now causes an island to be emitted in the return-call.clif test, which are the changes for that test in this PR.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 19:05):

afonso360 requested cfallin for a review on PR #8850.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 19:05):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #8850.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 19:11):

cfallin submitted PR review:

Thanks! Suggestion for a test but otherwise LGTM.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 19:11):

cfallin submitted PR review:

Thanks! Suggestion for a test but otherwise LGTM.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 19:11):

cfallin created PR review comment:

I agree it's theoretically hard to enumerate all the different possible Insts and programmatically test the worst case -- but could we add a test at least that manually constructs what we think is the worst-case Inst (return_call_indirect` with all clobbers set, etc) and asserts its size is 172? Size tests like this for large pseudoinsts plus a social norm of adding them for new large pseudoinsts will at least give partial coverage for this issue I think...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 20:14):

afonso360 updated PR #8850.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 20:19):

afonso360 has enabled auto merge for PR #8850.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 20:31):

afonso360 updated PR #8850.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 20:52):

alexcrichton commented on PR #8850:

I'm not entirely sure why it bisects to the ZiCond PR (https://github.com/bytecodealliance/wasmtime/pull/8695)

In retrospect this is my fault. The test case in #8847 uses has_zicond so during bisection I marked "unknown feature has_zicond" as "good" which pointed to the ZiCond PR. I suspect if I had just removed the zicond feature itself I would have found a different point of bisection.

Sorry about that, should have dug in a bit further myself! I naively assumed that the bug was in the zicond instructions added but in retrospect I see how that doesn't make sense

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

afonso360 merged PR #8850.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 20 2024 at 22:11):

cfallin edited PR review comment.

Last updated: Nov 22 2024 at 16:03 UTC