GeezerVison

Please fix the comment.

May I ask to keep (redundantly) the INSTR_IP(...) argument in all cases? It's helpful for debugging -- the 64-bit hex operand is useless, but the code-relative offset can be related directly to the output from dis, and I regularly do that.

If you worry about the cost of computing INSTR_IP() when no debugging is selected, maybe define the macro differently (or define another macro)?

Also, I think the operand may have to be printed in hex (both here and in executor.c) -- debug output like 106858786520180 is not very helpful.

I think we will ultimately want to change from instruction pointer back to the offset (as we had in 3.9 and earlier), so let's keep the redundant (for now) macro.

The reason for wanting the offset, not the pointer is that we expect writes to frame->instr_ptr to be rare, thanks to optimization, most reads actually want the offset, and it means we only need a 32 bit operand (less copying and cache pressure).

Interesting. Because you added an assignment to frame->prev_instr to _PUSH_FRAME, you don't need this dummy SAVE_IP any more. Same when bailing on version mismatch below and ditto if func == NULL. Right?

I had to look twice before understanding how we even get here. :-) I think we could use a comment here explaining that here func == NULL. (And/or a DPRINT() call?)

I'd be curious what @markshannon thinks of this change. It makes me think that the "abstractions" embodied by these LOAD/STORE macros are not as solid as we thought. And I wonder if the line that sets prev_instr should also be a macro. (Note that the _SP macros don't actually differ by tier.)

This seems good to me. LOAD_IP should be a no-op in tier 2 execution. The IP should be virtual.

(In this context "virtual" means tracked by the region selector (translate_bytecode_to_trace) not at runtime)

The _SP may not differ by tier, but the copy-and-patch compiler will need to treat them differently from the tier 2 interpreter.

In uops debug mode (PYTHONUOPSDEBUG=2 or higher) this would (confusingly, IMO) print DEOPT: [Opcode ...]. Doesn't that bother you? Or do you consider EXIT_TRACE as just another form of deoptimization?

It definitely is annoying. Actual deoptimizations are rare, but exiting a trace is common. So I get a lot of log spam containing endless repetitions of

DEOPT: [Opcode 300, operand 0]

This is tier 2 only, so it doesn't matter much what the implementation is. I can see it changing quite a few times in the near future.
Whatever it is, it shouldn't have the context dependent frame->prev_instr--; // Back up to just before destination

See my comment in ceval_macros.c -- perhaps this should also be a macro?

This is unnecessary, frame->prev_instr is set during frame creation.

And if we want to reuse this code for SEND we don't want to be messing with frame->prev_instr

This is unnecessary, frame->prev_instr is set during frame creation.

It's set to an index of -1 (since the frame hasn't started yet), which is invalid. So we either need to set it like this, or at least do an increment.