ZJIT: Avoid spilling locals/stack slots that have already been spilled and not modified yet

[k0kubun]

We spill every local and stack slot on gen_spill_*(), but we shouldn't need to spill the same things multiple times.

[tekknolagi]

What level of the stack are you planning on doing this?

[k0kubun]

What do you mean by "what level"? I don't know of any level in the stack used by an ISEQ.

[tekknolagi]

HIR, LIR, etc. The computing stack, so to speak

[k0kubun]

Basically I intend to change FrameState::stack and FrameState::locals from Vec<InsnId> to Vec<Option<InsnId>> or Vec<TwoStateEnumThatMayHaveInsnId> in the HIR. Then spill code for LIR will change for each stack/local slot, depending on the variant of the Option/enum in the FrameState.

[tekknolagi]

Can we do this in an LIR-only way? Maybe LIR can have its own FrameState-alike created from an HIR FrameState? This seems like a thing that should not concern HIR

[k0kubun]

I was just realizing it's kinda hard to do this in FrameState at the moment since the spill decision is made after making HIR. I see two possible options:

1. Let HIR have spill instructions and manage the spill state in FrameState
2. Leave FrameState as is and let codegen.rs manage the spill state

You seem to like (2) and I'm fine with that too, guessing HIR optimization passes do not really benefit from having those insns.

[k0kubun]

Well, but part of the reason why I filed this was because I thought YARVINSN_getlocal should behave differently depending on the spill state for #717, which can be modified by YARVINSN_send.

The enum should have three states for locals:

1. Not spilled (VM stack: not written, VReg: written): locals written by JIT code before any non-leaf call
2. Spilled (VM stack: written, VReg: written): locals written by JIT code after non-leaf calls, or method parameters on entry
3. Not loaded (VM stack: written, VReg: not written): locals that could be modified by blockiseq on send

Locals in the (3) category need to be reloaded by Insn::GetLocal. If HIR is not aware of it and inserts no Insn::GetLocal for it, LIR has to reload every local in the (3) state right after Insn::Send, even if they may not be read by YARVINSN_getlocal after that, which doesn't seem ideal.

So I think we do need a change in FrameState if we want to optimize ISEQs with blockiseq sends.

[XrXr]

This feels pretty HIR to me because it has interplay with putting the method entry stubs that loads all parameters from VM stack memory into HIR. We want to only load the parameters that are used, so the "Not loaded" state in #718 (comment) feels like the type of thing that requires some analysis esp. with loops?