Skip to content

ZJIT: Add support for putspecialobject #13565

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ZJIT: Add support for putspecialobject #13565

wants to merge 2 commits into from

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Jun 9, 2025

No description provided.

@matzbot matzbot requested a review from a team June 9, 2025 13:40
@st0012 st0012 force-pushed the zjit-support-putspecialobject branch 2 times, most recently from 811e6c2 to 14440bd Compare June 9, 2025 13:47
st0012
st0012 commented Jun 9, 2025
View reviewed changes
Copy link
Member Author

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

So the current tests cover 2 special object types: VMCORE and CBASE. But I'm not sure how to generate CONST_BASE 🤔

vm_insnhelper.c Outdated
@@ -5550,6 +5550,13 @@ vm_get_special_object(const VALUE *const reg_ep,
}
}

// ZJIT implementation is using the C function
// and needs to call a non-static function
VALUE rb_vm_get_special_object(const VALUE *reg_ep, enum vm_special_object_type type)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if exposing static VM functions for jit is allowed as I didn't see many examples. But we seems to do rb_vm_concat_array for YJIT?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is allowed

Copy link
Contributor

Choose a reason for hiding this comment

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

Although maybe we just remove static sometimes? What do you think @XrXr / @k0kubun ?

Copy link
Member

Choose a reason for hiding this comment

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

I do the wrapper function one when I hope its performance impact on the interpreter will be smaller than just removing static (because static functions may be inlined), but they may actually result in the same compilation result. If you're unsure, just try both and pick whichever you think is better.

Also, like other functions in this file, CRuby uses this style:

Suggested change
VALUE rb_vm_get_special_object(const VALUE *reg_ep, enum vm_special_object_type type)
VALUE
rb_vm_get_special_object(const VALUE *reg_ep, enum vm_special_object_type type)

tekknolagi
tekknolagi approved these changes Jun 9, 2025
View reviewed changes
Copy link
Contributor

@tekknolagi tekknolagi left a comment

Choose a reason for hiding this comment

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

lgtm! if you want to get fancy, you can add something to infer_type that in the case of VM_SPECIAL_OBJECT_VMCORE does Type::from_value(rb_mRubyVMFrozenCore)

vm_insnhelper.c Outdated
@@ -5550,6 +5550,13 @@ vm_get_special_object(const VALUE *const reg_ep,
}
}

// ZJIT implementation is using the C function
// and needs to call a non-static function
VALUE rb_vm_get_special_object(const VALUE *reg_ep, enum vm_special_object_type type)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is allowed

@tekknolagi
Copy link
Contributor

So the current tests cover 2 special object types: VMCORE and CBASE. But I'm not sure how to generate CONST_BASE 🤔

search for CONST_BASE in prism_compile.c -- looks like there are a couple of uses

@tekknolagi
Copy link
Contributor

tekknolagi commented Jun 9, 2025 •
edited
Loading

for I in []; end

EDIT: Or even Foo = 5

tekknolagi
tekknolagi reviewed Jun 9, 2025
View reviewed changes
zjit/src/hir.rs Outdated
impl From<u32> for SpecialObjectType {
fn from(value: u32) -> Self {
match value {
1 => SpecialObjectType::VMCore,
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, since rb_mRubyVMFrozenCore is used expressly by YJIT already, we should be able to do the same. So please instead compile the 1 case into a Const instruction with rb_mRubyVMFrozenCore

tekknolagi
tekknolagi reviewed Jun 9, 2025
View reviewed changes
test/ruby/test_zjit.rb Outdated
@@ -610,6 +610,18 @@ def test() = @foo = 1
}
end

def test_putspecialobject
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please try to cover each subcase of putspecialobject, if possible, in the execution tests?

k0kubun
k0kubun reviewed Jun 9, 2025
View reviewed changes
@st0012 st0012 force-pushed the zjit-support-putspecialobject branch from 14440bd to 04815b6 Compare June 10, 2025 20:45
@st0012
Copy link
Member Author

st0012 commented Jun 10, 2025

@k0kubun @tekknolagi I'm having issues adding test cases for ConstBase, which requires either
Foo = 1 or class Module::Bar; end (class defined under a module). And ZJIT unit tests expects the target instruction to be inside a method, but neither of these can be performed inside methods.

For integration tests, I'm trying

Foo = 1

def test
  Foo
end

test

which I'd expect to have hirs like:

$ build/miniruby --zjit-dump-hir --zjit-call-threshold=1 test.rb
HIR:
fn <main>:
bb0(v0:BasicObject):
  v2:Fixnum[1] = Const Value(1)
  v3:BasicObject = PutSpecialObject ConstBase
  SideExit

HIR:
fn test:
bb0(v0:BasicObject):
  v2:BasicObject = GetConstantPath 0x16fa15318
  Return v2

But the test fails with

TestZJIT#test_putspecialobject_const_base [/Users/hung-wulo/src/github.com/Shopify/ruby/test/ruby/test_zjit.rb:633]:
exited with status 6
stderr:
ZJIT: gen_function: unexpected insn Send { self_val: InsnId(5), call_info: CallInfo { method_name: "lambda" }, cd: 0x1536c4e00, blockiseq: 0x1031d21f0, args: [], state: InsnId(6) }
Failed to compile insn: v7 Send { self_val: InsnId(5), call_info: CallInfo { method_name: "lambda" }, cd: 0x1536c4e00, blockiseq: 0x1031d21f0, args: [], state: InsnId(6) }
ZJIT: gen_function: unexpected insn GetConstantPath { ic: 0x1536c4570 }
Failed to compile insn: v2 GetConstantPath { ic: 0x1536c4570 }
ZJIT: gen_function: unexpected insn SideExit { state: InsnId(2) }
Failed to compile insn: v3 SideExit { state: InsnId(2) }
ZJIT: gen_function: unexpected insn GetConstantPath { ic: 0x1536c45f0 }
Failed to compile insn: v2 GetConstantPath { ic: 0x1536c45f0 }
ruby: ZJIT has panicked. More info to follow...

thread '<unnamed>' panicked at zjit/src/codegen.rs:89:9:
Failed to compile: test@-e:7
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5

I'm not sure how those additional insn are generated 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tekknolagi tekknolagi tekknolagi approved these changes

@k0kubun k0kubun k0kubun left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@st0012 @tekknolagi @k0kubun