-
Notifications
You must be signed in to change notification settings - Fork 5.4k
ZJIT: Add a simple HIR validator #13780
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
base: master
Are you sure you want to change the base?
Conversation
cc @tekknolagi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start--looking good
zjit/src/hir.rs
Outdated
} | ||
|
||
fn validate_block_uses(&self) { | ||
// TODO(kenjin): Need to validate all uses and kills are properly gen-ed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you be doing this in this PR? If not, let's remove this function for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this function, will be done in another follow-up, as it might be a little slow to run.
Sorry: new to ruby contributing. Should I force push or will you squash merge at the end? |
I can squash at the end. If you have multiple logical commits that you would like preserved, you should force push and I can rebase |
Nope I don't. Please squash and overwrite the commit message with your own at the end. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good!
If you have time, please add some tests for this, but it's not dire. I will otherwise merge tonight or Monday |
I would add tests, but I'm not too sure how to at the moment. We'd basically need to pass a malformed iseq to the translation function and have it fail on validation right? Not sure how to construct an iseq in Rust in the first place considering |
No, we'd construct the HIR manually---not from an ISEQ |
Ah ok, I was under the impression you wanted an integration test for |
Oh nevermind, I just found out |
This comment has been minimized.
This comment has been minimized.
The failures look to be flakiness of Ractor tests. This PR doesnt touch Ractors at all so I'm not concerned. |
This PR adds a simple validator for ZJIT's HIR.
See issue Shopify#591