ZJIT: Pass self through basic block params #13529
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c41b83b to
84d0f24
Compare
Co-authored-by: Max Bernstein <tekknolagi@gmail.com>
tekknolagi
requested changes
Jun 5, 2025
zjit/src/hir.rs
Outdated
| let val = function.push_insn(function.entry_block, Insn::Test { val: param }); | ||
| function.infer_types(); | ||
| assert_bit_equal(function.type_of(val), types::CBool); | ||
| assert_bit_equal(function.type_of(val), Type::from_cbool(false)); |
There was a problem hiding this comment.
Why did this change? We shouldn't know that it's false
There was a problem hiding this comment.
This is still weird to me :/
There was a problem hiding this comment.
Aha, we need to do
function.param_types.push(types::BasicObject); // selfThere was a problem hiding this comment.
Thanks for pointing out the fix. Yeah I was looking into this and it had Empty and is_known_falsy() returned true (?). Pushed the fix at 5a543a6.
I wish we had less (ideally, no) synthetic tests like this. It seems to test something unreal and requires non-trivial effort for debugging it. The more integrated, the easier to maintain.
tekknolagi
requested changes
Jun 5, 2025
|
I can't push a commit but I think you want something like commit d4d83a0d74b8ffe406ae9e5acd38b2a53f6829be (HEAD -> zjit-self-param)
Author: Max Bernstein <max@bernsteinbear.com>
Date: Thu Jun 5 16:44:55 2025 -0400
Only push self param for non-entry blocks
diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs
index 1d26c99964..2711c7561b 100644
--- a/zjit/src/hir.rs
+++ b/zjit/src/hir.rs
@@ -1989,6 +1989,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result<Function, ParseError> {
// item between commas in the source increase the parameter count by one,
// regardless of parameter kind.
let mut entry_state = FrameState::new(iseq);
+ fun.push_insn(fun.entry_block, Insn::Param { idx: SELF_PARAM_IDX });
fun.param_types.push(types::BasicObject); // self
for local_idx in 0..num_locals(iseq) {
if local_idx < unsafe { get_iseq_body_param_size(iseq) }.as_usize() {
@@ -2012,11 +2013,14 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result<Function, ParseError> {
while let Some((incoming_state, block, mut insn_idx)) = queue.pop_front() {
if visited.contains(&block) { continue; }
visited.insert(block);
- let self_param = fun.push_insn(block, Insn::Param { idx: SELF_PARAM_IDX });
- let mut state = if insn_idx == 0 { incoming_state.clone() } else {
+ let (self_param, mut state) = if insn_idx == 0 {
+ (fun.blocks[fun.entry_block.0].params[0], incoming_state.clone())
+ } else {
+ assert_eq!(fun.blocks[block.0].params.len(), 0);
+ let self_param = fun.push_insn(block, Insn::Param { idx: SELF_PARAM_IDX });
let mut result = FrameState::new(iseq);
let mut idx = 1;
- for _ in 0..incoming_state.locals.len() {
+ for _ in incoming_state.locals {
result.locals.push(fun.push_insn(block, Insn::Param { idx }));
idx += 1;
}
@@ -2024,7 +2028,7 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result<Function, ParseError> {
result.stack.push(fun.push_insn(block, Insn::Param { idx }));
idx += 1;
}
- result
+ (self_param, result)
};
// Start the block off with a Snapshot so that if we need to insert a new Guard later on
// and we don't have a Snapshot handy, we can just iterate backward (at the earliest, to |
tekknolagi
reviewed
Jun 5, 2025
Co-authored-by: Max Bernstein <tekknolagi@gmail.com>
tekknolagi
approved these changes
Jun 5, 2025
zjit/src/hir.rs
Outdated
| let val = function.push_insn(function.entry_block, Insn::Test { val: param }); | ||
| function.infer_types(); | ||
| assert_bit_equal(function.type_of(val), types::CBool); | ||
| assert_bit_equal(function.type_of(val), Type::from_cbool(false)); |
There was a problem hiding this comment.
This is still weird to me :/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the storage of
selffromcfp->selfto the first basic block argument to use a register.Currently, ZJIT basic blocks are not allowed to refer to an argument of another basic block because the register allocator uses a fixed register for each basic block argument index, which would cause a conflict for such a reuse. I left a TODO comment in
as_args()to fix it when we make the register allocator more flexible.