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 thereturn_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.
afonso360 requested cfallin for a review on PR #8850.
afonso360 requested wasmtime-compiler-reviewers for a review on PR #8850.
cfallin submitted PR review:
Thanks! Suggestion for a test but otherwise LGTM.
cfallin submitted PR review:
Thanks! Suggestion for a test but otherwise LGTM.
cfallin created PR review comment:
I agree it's theoretically hard to enumerate all the different possible
Inst
sand 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...
afonso360 updated PR #8850.
afonso360 has enabled auto merge for PR #8850.
afonso360 updated PR #8850.
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 featurehas_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
afonso360 merged PR #8850.
cfallin edited PR review comment.
Last updated: Dec 23 2024 at 12:05 UTC