ZJIT: Add newrange support #13505
ZJIT: Add newrange support #13505
Conversation
This comment has been minimized.
This comment has been minimized.
a093af0 to
d73b3ab
Compare
| flag: u32, | ||
| state: &FrameState, | ||
| ) -> lir::Opnd { | ||
| asm_comment!(asm, "call rb_range_new"); |
There was a problem hiding this comment.
Nit: this should go by the ccall
There was a problem hiding this comment.
In gen_array_dup it also uses call rb_ary_resurrect while doing asm.ccall. Should we be consistent and update that and other occurrences like this to use ccall {instruction name}?
There was a problem hiding this comment.
Yeah we should probably move the comments. I think it is a quirk of the PC saving being added later
zjit/src/hir.rs
Outdated
| @@ -334,6 +334,7 @@ pub enum Insn { | |||
| NewArray { elements: Vec<InsnId>, state: InsnId }, | |||
| /// NewHash contains a vec of (key, value) pairs | |||
| NewHash { elements: Vec<(InsnId,InsnId)>, state: InsnId }, | |||
| NewRange { low: InsnId, high: InsnId, flag: u32, state: InsnId }, | |||
There was a problem hiding this comment.
Please convert flag into a Rust-style enum if it's just Inclusive|Exclusive
There was a problem hiding this comment.
oh, it's a bool called exclude_end. i think enum { Inclusive, Exclusive } makes sense here
There was a problem hiding this comment.
Last, we don't have this yet, but in a stack @tenderlove and I are working on, we have an ObjectAlloc instruction, so we can probably eventually split this into ObjectAlloc+FieldSet+FieldSet or something similar. But again, these instructions don't exist now
There was a problem hiding this comment.
Do you mean we will be splitting new_something instructions into a combination of field set instructions + ObjectAlloc?
|
Oh also I think PR/comment should be prefixed with "ZJIT:" like "ZJIT: Parse newrange into HIR" |
27447c4 to
fb9f3d4
Compare
fb9f3d4 to
207ee93
Compare
cc @tekknolagi