ZJIT: NewRangeFixnum instruction #14409
Merged
+233
−1
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
|
Could you take a look at this draft PR? Want to make sure this is the right direction. |
|
I'm on vacation for the next two weeks but someone else on the team can give it a look! |
|
cc @ruby/jit |
st0012
reviewed
Sep 1, 2025
zjit/src/hir.rs
Outdated
| @@ -2082,6 +2135,11 @@ impl Function { | |||
| worklist.push_back(high); | |||
| worklist.push_back(state); | |||
| } | |||
| &Insn::NewRangeFixnum { low, high, state, .. } => { | |||
There was a problem hiding this comment.
This can be stacked with NewRange like
&Insn::NewRangeFixnum { low, high, state, .. } |
&Insn::NewRange { low, high, state, .. } => {
worklist.push_back(low);
worklist.push_back(high);
worklist.push_back(state);
}
zjit/src/hir.rs
Outdated
| @@ -6548,6 +6608,116 @@ mod opt_tests { | |||
| "); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_optimize_range_fixnum_inclusive_literals() { | |||
| eval(r#" | |||
There was a problem hiding this comment.
I think none of the new tests new r# as there's no character needs escaping.
| def test(a) | ||
| (a...10) | ||
| end | ||
| test(2); test(3) |
There was a problem hiding this comment.
Can you add these cases to test/ruby/test_zjit.rb too as integration tests? Remember to add call_threshold: 2 option so we actually run the optimzied HIR.
k0kubun
reviewed
Sep 2, 2025
k0kubun
approved these changes
Sep 2, 2025
st0012
approved these changes
Sep 2, 2025
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.
Context
Closes Shopify#710
Changes
HIR
NewRangeFixnumthat mirrorsNewRangebut is specialized for Fixnum endpoints.NewRange, which may call<=>).Codegen
gen_new_range_fixnum, wired to the new instruction.gen_prepare_call_with_gc(asm, state)since range allocation may trigger GC, but does not call Ruby methods.rb_range_new.Optimization
optimize_rangespass: conservatively rewritesNewRangeintoNewRangeFixnumwhen one or both ends are statically Fixnum literals (with guards added for the dynamic side if needed).At this point I haven’t added rewrites for parameter–parameter ranges.
From looking at how
opt_plusworks, it seems to rely on profiling info, but I don’t fully understand whether that same info is available fornewrange.Since I’m still new to this codebase (and to low-level Rust in general), I decided to keep the optimization conservative for now and get maintainer feedback before broadening it.