ZJIT: NewRangeFixnum instruction #14409
Conversation
|
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 |
zjit/src/hir.rs
Outdated
| 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
|
|
||
| #[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.
| fn test_optimize_range_fixnum_exclusive_literals() { | ||
| eval(" | ||
| def test() | ||
| (1...2) |
There was a problem hiding this comment.
You may have noticed that this doesn't produce a New range instruction. For that you probably need to use a local variable (makes it harder for the bytecode compiler to reason about, but the JIT sees through it). If you have time, please rewrite these tests.
There was a problem hiding this comment.
Will handle this later this week, along with you other comment.
| self.insn_types[repl.0] = self.infer_type(repl); | ||
| } else if low_is_fix { | ||
| // Only left is fixnum => guard right | ||
| let high_fix = self.coerce_to_fixnum(block, high, state); |
There was a problem hiding this comment.
There's some code above to do this for you: see arguments_likely_fixnums and coerce something or other
There was a problem hiding this comment.
Didn't found arguments_likely_fixnum to be applicable here, unless we change how this optimization works.
Profiling info is not enabled for NewRange, so this optimization fires if either end of the range is a fixnum, without looking at profiling info. For example:
def test(a)
(1..a)
endIs a case where the optimization fires even if we don't have profiling info for a, because the start of the range is a Fixnum literal.
We could enable profiling for NewRange and just use arguments_likely_fixnum as you said. In effect, this would make this more similar to other Fixnum operations.
I submitted a draft PR #14607. Would like you to take a look before marking it ready for review.. Decided to just mark it as ready.
## Context #14409 #14409 (comment) >You may have noticed that this doesn't produce a New range instruction. For that you probably need to use a local variable (makes it harder for the bytecode compiler to reason about, but the JIT sees through it). If you have time, please rewrite these tests. #14409 (comment) >There's some code above to do this for you: see arguments_likely_fixnums and coerce something or other ## Changes ### Refactor tests Modify tests force the usage of `NewRangeFixnum` instruction and make tests names more consistent. ### Refactor optimize ranges Didn't found `arguments_likely_fixnums` to be applicable unless we enable profiling for `NewRange` and change the criteria of when the optimization fires. But there were other ways to clean up the code. Simplify the logic and move it to `type_specialize`. Deleted the standalone `optimize_ranges` function.
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.