ZJIT: unify hir::Insn::{Send,SendWithoutBlock}

[chancancode]

After #15911, we largely apply the same treatment to these two variants everywhere. In type_specialize, there are two ~100 LOC match arms that shared 90% of the code. Unifying these allowed the code to be shared more easily.

---

Note:

In the interest of preserving existing semantics and keeping the diff small for this initial PR, YARVINSN_send can currently produce:

Insn::Send { blockiseq: Some(null_ptr), .. }

This basically happens for cases like foo(&:bar) where YARV opted not to specialize into YARVINSN_opt_send_without_block. Inside type_specialize, we check for these conditions (indirectly, not by matching Some(null_ptr)) and bail, so they ended up remaining Insn::Send throughout, and goes through rb_vm_send which allows for blockiseq: null_ptr.

There isn't a great reason to do this, as far as I can tell. At minimum, it seems like a pretty backhanded way to thread this, but also nothing seemed to care, as we can re-derive the information via ci flags.

It was a small change to make this case blockiseq: None instead. For the most part, everything works (some cosmetic snapshot diffs), except that we need to somehow avoid routing these cases through rb_vm_opt_send_without_block.

Options:

1. Accept that all unspecialized Send go through rb_vm_send

2. Check ci flags in gen_send to decide between rb_vm_send or rb_vm_opt_send_without_block

3. Bring back Insn::SendWithoutBlock, but much narrower – as a possible specialization we emit in type_specialize, rather than as an input to the funnel like it was previously

Either way, that can be done in a follow-up PR.

---

Follow-up tasks:

1. Unify reduce_send_to_ccall/reduce_send_without_block_to_ccall

2. Address the Some(null_ptr) situation above

3. Rename/merge the stats/counters/fallback reasons, if desired

Partially addresses Shopify#941 (TODO: optimize_c_calls)

[chancancode]

Fixed the syntax error on older rustc. Questions for follow-ups

1. Do we want to rename/merge the counters/fallback reasons? (SendWithBlockXXX -> SendXXX)
2. Which, if any, of the options above do you prefer for the foo(&:bar) case?

[chancancode]

This is where the Some(null_ptr) is constructed – see is_null() check below

[chancancode]

For the review, the mental model of the current state is that blockiseq is really an enum of three possible states: Some(blockiseq), Some(null) and None (previously SendWithoutBlock).

Mechanically:

• previous SendWithoutBlock arms became Send { blockiseq: None } arms
• previous Send arms became Send { blockiseq: Some(_) } arms
• within the Send arms, wherever we previously cared about is_null(), we still need to keep checking.

As long as we do all three, the before/after should be exactly equivalent.

Don't think that's a good state to leave things in the long term, but it kept the diff 1:1 for the initial review, and I'll take directions on how we want to further refactor/evolve this afterwards.

[XrXr]

Re: Some(null_ptr) situation, the Option is acting as signal for the provenance of the HIR instruction. It is backhanded way to represent this.

Options: Accept that all unspecialized Send go through rb_vm_send
Check ci flags in gen_send to decide between rb_vm_send or rb_vm_opt_send_without_block
Bring back Insn::SendWithoutBlock, but much narrower – as a possible specialization we emit in type_specialize, rather than as an input to the funnel like it was previously

The there's strict correspondence between the fallback function used and the specific opcode we need to maintain. That rules out the first two options. We also want to stick to just one send instruction.

We could look at the frame state and find the interpreter opcode through the PC, but it feels better to parse this onto the HIR instruction as a field. So, raw pointer for blockiseq, and replace the Option with an explicitly named boolean field.

This basically happens for cases like foo(&:bar) where YARV opted not to specialize into YARVINSN_opt_send_without_block.

This is VM_CALL_ARGS_BLOCKARG; it's still sending with a block, just not through blockiseq. So blockiseq is nil in these cases.

[chancancode]

Working on finding/updating the test case that actually catch the regression, please don't merge yet

Updated now, should be ready to merge.

Context: the original runtime crash I eventually found had to do with passing a bogus block handler to Enumerable, but later I realized the issue can be simplified – it doesn't actually matter that there is or isn't a block, the issue is that with specialized_instruction: false, everything goes through YARVINSN_send and gets Some(null), so anything that goes into the cfunc path will get a bogus block handler.

To get it to crash, you'd have to do something with that block handler (yielding), but for the purpose of the hir snapshot test, just having/not having a block in the resulting hir is sufficient to catch the regression.

I neglected to update the comment after the long session and ended up confusing myself on the second read. Updated the comment. All good now.

[chancancode]

Note: these are discussions for post-merge follow-up tasks

Re: Some(null_ptr) situation, the Option is acting as signal for the provenance of the HIR instruction. It is backhanded way to represent this.

Options: Accept that all unspecialized Send go through rb_vm_send
Check ci flags in gen_send to decide between rb_vm_send or rb_vm_opt_send_without_block
Bring back Insn::SendWithoutBlock, but much narrower – as a possible specialization we emit in type_specialize, rather than as an input to the funnel like it was previously

The there's strict correspondence between the fallback function used and the specific opcode we need to maintain. That rules out the first two options. We also want to stick to just one send instruction.

We could look at the frame state and find the interpreter opcode through the PC, but it feels better to parse this onto the HIR instruction as a field. So, raw pointer for blockiseq, and replace the Option with an explicitly named boolean field.

This basically happens for cases like foo(&:bar) where YARV opted not to specialize into YARVINSN_opt_send_without_block.

This is VM_CALL_ARGS_BLOCKARG; it's still sending with a block, just not through blockiseq. So blockiseq is nil in these cases.

@XrXr I think given my last comment, I'm not sure the provenance (I took that to mean "which YARV instructions did it originate from") is what we actually want to capture or a reliable/useful signal.

Here is my current understanding based on reading code and interpreting your comment. Please correct me if I'm getting some details wrong:

What we really want to capture here: no block passed, passing an iseq block, passing a non-iseq block. Can we use an BlockArg enum to signal there are three possible states, not four as bool + nullable ptr combo would imply? (Technically, there are still four states in the enum, since we aren't using NonNull, but we can let that one slide.)

With specialized_instruction, we implicitly rely on YARV for the triage, so:

• YARV_opt_send_without_block: currently blockiseq: None, proposed blockarg: None
• YARV_send + blockiseq non-null = currently blockiseq: Some(non_null_iseq_ptr), proposed blockarg: IseqBlock(IseqPtr)
• YARV_send + null = currently blockiseq: Some(null), proposed blockarg: NonIseqBlock (better names welcomed)

This is fine.

But it seems like we are also required to handle specialized_instruction: false (I originally had all zjit tests working but had to debug the specialized_instruction: false crash).

This is where we were supposed to do the triage in the YARV_send handler, but didn't. IMO this is the actual bug.

Somewhat coincidentally, it mostly didn't turn out to be a big deal – for the most part, type_specialize today has the emergent behavior of doing the same triage anyway. Because we mostly don't support the "non-iseq-block-arg passed" cases, by bailing out, we effectively re-did the triage and masked/avoided the bug.

Except in the case of the cfunc path, where it happens to propagate the untriaged blockiseq: Some(IseqPtr), and because the type matched, I initially removed the normalization and passed through the Some(null_ptr) to gen_ccall_with_frame where it wasn't expected.

---

So there are really two things:

1. How do we represent the the possible block arg states – if my current understanding is right then I think we are mostly aligned, my main ask/question is about using an enum with named variants, instead of two unrelated fields

2. Where & how we do the triage, this one kind of flew under the radar initially. It seems like, for correctness in the face of specialized_instruction: false, we have to defensively parse the ci flags in the YARV_send handler unconditionally? Unless there is some way to tell which mode we are in an do that conditionally.

[XrXr]

Using an enum sounds good to me. In case of BLOCKARG, it references a value, so we should take care to wire up the SSA and frame state AI correctly. For naming, maybe call the field block and the variants None, Literal(IseqPtr), BlockArg(InsnId).

It seems like, for correctness in the face of specialized_instruction: false, we have to defensively parse the ci flags in the YARV_send handler unconditionally?

Yes. We need to do this anyways because presence of BLOCKARG impacts the VM stack size post-send.

I'm not sure the provenance ... is what we actually want to capture or a reliable/useful signal.

This is just for in case of falling back and we actually need to execute the Send instruction. Since the fallback functions are 1:1 interpreter opcode handlers we need to maintain the correspondence. If an YARV_send come in with no blockiseq and no BLOCKARG, we still should be using rb_vm_send to run it. This is an orthogonal concern to modeling whether and what block is passed. We should handle both.

[chancancode]

Sounds good to me, happy to do that after merge, I think this current PR preserved the current imperfect state exactly as it was before

[chancancode]

@XrXr sorry, one more Q: do we want to leave the fallback reasons/counters as SendWithoutBlock* or merge them?

[XrXr]

Let's merge them. I can't think of a situation where the distinction was actually useful and IIRC Kokubun mentioned wanting to merge them.