ZJIT: Standardize method dispatch insns' recv field
#15334
Conversation
st0012
changed the title
recv field
|
lobsters assert failures look relevant. |
st0012
force-pushed
the
zjit-standardize-ccall-hir-fields
branch
from
1475e1f to
528f57d
Compare
|
@XrXr Fixed! |
st0012
force-pushed
the
zjit-standardize-ccall-hir-fields
branch
from
528f57d to
7f98741
Compare
| Insn::CCallWithFrame { cd, state, args, .. } if args.len() > C_ARG_OPNDS.len() => | ||
| Insn::CCall { cfunc, recv, args, name, return_type: _, elidable: _ } => gen_ccall(asm, *cfunc, *name, opnd!(recv), opnds!(args)), | ||
| // Give up CCallWithFrame for 6+ args since asm.ccall() supports at most 6 args (recv + args count). | ||
| Insn::CCallWithFrame { cd, state, args, .. } if args.len() + 1 > C_ARG_OPNDS.len() => |
There was a problem hiding this comment.
More experienced people than I should probably confirm that I'm on the right track - but maybe a test to show this behaviour for > 6 args could be useful.
There was a problem hiding this comment.
In Ruby core itself, there's no method that take 6 parameters that can trigger this AFAIK. Fiber::Scheduler has methods that take up to 5, but they're not enough either.
There was a problem hiding this comment.
It's a good call tho. I'll add a comment to explain this 🙂
aidenfoxivey
left a comment
aidenfoxivey
left a comment
There was a problem hiding this comment.
Gave it a look over, seems like a reasonable change to me. I added a comment.
st0012
force-pushed
the
zjit-standardize-ccall-hir-fields
branch
from
7f98741 to
7831770
Compare
This comment has been minimized.
This comment has been minimized.
st0012
force-pushed
the
zjit-standardize-ccall-hir-fields
branch
from
7831770 to
91bb08f
Compare
- Add `recv` field to `CCall` and `CCallWithFrame` so now all method dispatch related instructions have `recv` field, separate from `args` field. This ensures consistent pointer arithmetic when generating code for these instructions. - Standardize `recv` field's display position in send related instructions.
st0012
force-pushed
the
zjit-standardize-ccall-hir-fields
branch
from
91bb08f to
0efa8ba
Compare
|
I don't think this is a good idea as-is. CCall is a general-purpose utility to call a C function. If we want to have IR for "call a Ruby C method without pushing a frame" then we should make that a separate IR instruction that has a So:
or instead,
|
Do you have examples of those?
In both cases, we already have receivers. So I don't see the hallucination you mentioned. |
|
@eregon is adding some in his getivar specialization PR, I think. But if we truly don't have this problem right now, then ok, fine. In general I think we should move toward PushFrame/PopFrame but less urgent |
|
For the ref, #15327 uses |
| | Insn::Send { recv, ref args, .. } | ||
| | Insn::SendForward { recv, ref args, .. } | ||
| | Insn::InvokeSuper { recv, ref args, .. } | ||
| | Insn::CCall { recv, ref args, .. } |
There was a problem hiding this comment.
This prevents calling rb_ivar_get_at_no_ractor_check(VALUE obj, attr_index_t index), so Insn::CCall* should probably not check the types of the args
ZJIT: Standardize C call related insn fields - Add `recv` field to `CCall` and `CCallWithFrame` so now all method dispatch related instructions have `recv` field, separate from `args` field. This ensures consistent pointer arithmetic when generating code for these instructions. - Standardize `recv` field's display position in send related instructions.
6 participants
recvfield toCCallandCCallWithFrameso now all method dispatch related instructions haverecvfield, separate fromargsfield. This ensures consistent pointer arithmetic when generating code for these instructions.recvfield's display position in send related instructions.