Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

riscv64: Update Inst::worst_case_size #8850

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

afonso360
Copy link
Contributor

👋 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 (#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 afonso360 added the cranelift:area:riscv64 Issues related to the RISC-V 64 backend. label Jun 20, 2024
@afonso360 afonso360 requested a review from a team as a code owner June 20, 2024 19:05
@afonso360 afonso360 requested review from cfallin and removed request for a team June 20, 2024 19:05
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Suggestion for a test but otherwise LGTM.

//
// We also have a test case that tests some other instructions which have
// the potential to generate a lot of bytes
172
Copy link
Member

@cfallin cfallin Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

@alexcrichton
Copy link
Member

I'm not entirely sure why it bisects to the ZiCond PR (#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

Merged via the queue into bytecodealliance:main with commit 67afe4d Jun 20, 2024
36 checks passed
@afonso360 afonso360 deleted the issue8847 branch June 20, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:riscv64 Issues related to the RISC-V 64 backend.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

riscv64: Panic using the Zicond extension
3 participants