GeezerVison

There are a few things about this family that make it unpleasant to convert.

First, the initial call stack can be either

[method, self, arg1, arg2, ...]

or

[NULL, callable, arg1, arg2, ...]

(This situation arises because the LOAD_ATTR instruction dynamically decides whether to produce one or the other.)

In the latter case we then introspect callable and if it is a bound method, we extract the im_func and im_self fields and convert to the former format -- but if the callable is not a bound method, we leave the NULL in place.

There is a macro is_method(stack_pointer, oparg) that, despite its lofty name, really just checks whether there's a NULL at the indicated position (using PEEK(oparg+1), it doesn't even use the stack_pointer argument). In the converted instructions I am modeling this as an input stack effect of the form thing1, thing2, args[oparg], and I replace the is_method() macro with thing1 != NULL. Everything else then gets a little awkward, because the thing we need to call in the end is either thing1 or thing2, and the number of arguments is either oparg or oparg+1. It also means that the first argument is not the start of the args array -- it is either that or one less.

Second, most specializations don't just call the function and produce a result. Because of Python function call inlining, most end up with DISPATCH_INLINE(), and because of that we can't leave the popping of the stack to the generated code (it would just be STACK_SHRINK(oparg+1)) -- we have to manually do this, and while we're at it we also either DECREF the arguments (including self, if present), or copy them into a new frame, depending on what we're calling.

All this leaves me with the feeling that the generator isn't a good match for the complexity of this instruction family, or I am missing a trick.

Maybe I should model the input stack effect as method, callable, args[oparg] and do something like

if (method != NULL) {
    callable = method;
    args -= 1;
    oparg += 1;
}

so that we can always call callable with oparg arguments starting at args. But we still need to recall whether we did this, so we know where to push the result.

Note that you linked this with the wrong issue.

@brandtbucher, @markshannon, any suggestions? (This is not all, I just would like some feedback on how to proceed with the remaining 12-odd specializations of CALL.)

Why not declare the interface as (null_or_func, func_or_self, arguments[oparg] -- result) and leave it at that?

We don't need the code generator to understand very bit of C code. We aren't implementing a C compiler.

TBH, I'm a little concerned that the code generator has become an end in itself, rather than being a means to an end.

TBH, I'm a little concerned that the code generator has become an end in itself, rather than being a means to an end.

Yeah, I agree. I'm not sure what we'll get out of it. Generating a tier 2 interpreter and supporting micro-ops does still seem like a worthy cause though. We should talk a bit about this on Monday.

Another thing where the code generator is getting in the way: CHECK_EVAL_BREAKER(). This may end up with goto handle_eval_breaker and at that point stack_pointer and next_instr should be updated. I guess the solution is to special-case this, and when found, move the call to just before DISPATCH(). We do the same thing with PREDICT(). (EDIT: Yeah, I should definitely do this, it makes things much simpler for most CALL* opcodes.)

I have made the requested changes; please review again.

I didn't literally do what you or Mark suggested; instead, I converted everything to the format (already used by many of these) (method, callable, args[oparg] -- res) with this little idiom:

int is_meth = method != NULL;
int total_args = oparg;
if (is_meth) {
    callable = method;
    args--;
    total_args++;
}

(Slight variations in a few cases.) I also removed most manual stack manipulation except when exiting via DISPATCH_INLINE(). I merged the latest main, so we now have (tadaaa) NO LEGACY INSTRUCTION DEFINITIONS LEFT.

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

Ah, I see you already ran the benchmarks in an earlier comment. Never mind!