Description

markshannon

opened on Feb 27, 2023

Issue body actions

Rather than emitting macros such as DISPATCH, POKE, TARGET etc, it would be useful if the code generator emitted something closer to plain C. Some macros will still be needed for portability.

Doing so would make the overhead in dispatch explicit and expose redundancies that can be eliminated.
For example, not all instructions need to save frame->prev_instr, but all do because the assignment is hidden in a macro.

Linked PRs

• GH-102305: Expand some macros in generated_cases.c.h #102309

Activity

1. markshannon

   added

   type-featureA feature request or enhancementA feature request or enhancement

   on Feb 27, 2023
2. markshannon

   assigned

   gvanrossum

   on Feb 27, 2023
3. gvanrossum commented on Feb 28, 2023

   

   gvanrossum

   on Feb 28, 2023

   Member

   More actions

   Expanding PEEK() and POKE() is easy. I think NEXTOPARG() is also straightforward, as is JUMPBY() -- I'll add those next.

   For TARGET the problem is that this is conditional on whether we use computed goto:

   #if USE_COMPUTED_GOTOS
   #  define TARGET(op) TARGET_##op: INSTRUCTION_START(op);
   #  define DISPATCH_GOTO() goto *opcode_targets[opcode]
   #else
   #  define TARGET(op) case op: TARGET_##op: INSTRUCTION_START(op);
   #  define DISPATCH_GOTO() goto dispatch_opcode
   #endif

   This references INSTRUCTION_START() which in turn is dependent on Py_STATS.

   I guess we could generate something like this for TARGET(op):

   #if !USE_COMPUTED_GOTO
       case op:
   #endif
       TARGET_##op:
           frame->prev_instr = next_instr++;
   #ifdef Py_STATS
           OPCODE_EXE_INC(op);
           if (_py_stats) _py_stats->opcode_stats[lastopcode].pair_count[op]++;
           lastopcode = op;
   #endif

   For DISPATCH() we could expand to something like this:

       {
           _Py_CODEUNIT word = *next_instr;
           opcode = word.op.code;
           oparg = word.op.arg;
   #ifdef LLTRACE
           if (lltrace) {
               lltrace_instruction(frame, stack_pointer, next_instr);
           }
   #endif
           assert(cframe.use_tracing == 0 || cframe.use_tracing == 255);
           opcode |= cframe.use_tracing OR_DTRACE_LINE;
   #if USE_COMPUTED_GOTO
           goto *opcode_targets[opcode]
   #else
           goto dispatch_opcode;
   #endif
       }

   But that's a lot of code to be repeated for each instruction (170 so far!) and I'm not sure it will enlighten the reader. Also, there currently are a few places that use DISPATCH() directly (plus a whole lot more using DISPATCH_INLINED()), and keeping the macro in sync with the code generator would be an unpleasant task for future maintainers.

   @markshannon Your thoughts on TARGET and DISPATCH?
4. gvanrossum commented on Feb 28, 2023

   

   gvanrossum

   on Feb 28, 2023

   Member

   More actions

   (FWIW I don't see much of a benefit in the expansion of DISPATCH(), but for TARGET() I can see the point -- in the future the generator can omit frame->prev_instr = next_instr++ if the instruction cannot produce an error. The Py_STATS code should probably be a single line calling a macro though, for flexibility.)
5. gvanrossum commented on Feb 28, 2023

   

   gvanrossum

   on Feb 28, 2023

   Member

   More actions

   If we're expanding TARGET(op) we should also expand PREDICTED(op). But PREDICT(op) is too complex to bother (it's almost a full copy of DISPATCH() -- see also faster-cpython/ideas#496).
6. markshannon commented on Feb 28, 2023

   

   markshannon

   on Feb 28, 2023

   MemberAuthor

   More actions

   Regarding, TARGET and BRANCH, it is only the label and goto that aren't portable,
   We already have DISPATCH_GOTO(), so keep that. Maybe reduce TARGET(name) to just the label:

   #if USE_COMPUTED_GOTO
           #define TARGET(op) TARGET_##op:
   #else
           #define TARGET(op) case op:
   #endif

   or to avoid confusion, add a new macro LABEL(op)?

   I'd leave PREDICT() for now, as we might be removing it.

   Reacted by Guido van Rossum
7. gvanrossum

   added a commit that references this issue on Feb 28, 2023

   GH-102305: Expand some macros in generated_cases.c.h (#102309)

   b5ff382
8. gvanrossum commented on Feb 28, 2023

   

   gvanrossum

   on Feb 28, 2023

   Member

   More actions

   A thought: should we also expand some of these macros where they occur in user code? E.g. JUMPBY(i) is still used frequently. Or should we just expand these in bytecodes.c?
9. carljm

   added a commit that references this issue on Feb 28, 2023

   Merge branch 'main' into inlinecomp2

   73dc0ed
10. iritkatriel

    added

    interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

    on Nov 27, 2023
11. gvanrossum

    closed this as completedon Mar 20, 2024

Issue actions