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

Cranelift: dedupe trap[n]z instructions #10004

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jan 14, 2025

This commit extends our existing support for merging idempotently side-effectful instructions that produce exactly one value to those that produce zero or one value, and marks the trap[n]z instructions as having idempotent side effects. This cleans up a lot test cases in our disas test suite, particularly those related to explicit bounds checks and GC.

As an aside, it seems like it should be easy to extend this to idempotently side-effectful instructions that produce multiple values as well, but I don't believe we have any such instructions, so I didn't bother.

@fitzgen fitzgen requested review from a team as code owners January 14, 2025 01:23
@fitzgen fitzgen requested review from cfallin and dicej and removed request for a team January 14, 2025 01:23
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Jan 14, 2025
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.

This is great -- thanks a bunch for working this out!

My only thoughts are for a few more comments and one more test, otherwise LGTM.

@fitzgen fitzgen enabled auto-merge January 14, 2025 18:15
This commit extends our existing support for merging idempotently side-effectful
instructions that produce exactly one value to those that produce zero or one
value, and marks the `trap[n]z` instructions as having idempotent side
effects. This cleans up a lot test cases in our `disas` test suite, particularly
those related to explicit bounds checks and GC.

As an aside, it seems like it should be easy to extend this to idempotently
side-effectful instructions that produce multiple values as well, but I don't
believe we have any such instructions, so I didn't bother.
@fitzgen fitzgen force-pushed the idempotent-trapz-trapnz branch from 66b96ae to f8800f1 Compare January 14, 2025 18:18
@fitzgen fitzgen added this pull request to the merge queue Jan 14, 2025
Merged via the queue into bytecodealliance:main with commit a88eb70 Jan 14, 2025
37 checks passed
@fitzgen fitzgen deleted the idempotent-trapz-trapnz branch January 14, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants