Uh oh!
There was an error while loading. Please reload this page.
gh-105481: add flags to each instr in the opcode metadata table, to replace opcode.hasarg/hasname/hasconst - #105482
Conversation
…, to replace opcode.hasarg/hasname/hasconst
gvanrossum
commented
Jun 7, 2023
I think so (unless all the flags are always false for all pseudo-ops, which I doubt is the case). In fact I think we should probably propose a solution for moving the canonical definition of the numeric opcode values before we move on this. (Other than that, this approach seems fine.) |
iritkatriel
commented
Jun 7, 2023
Also I don’t know how to identify the jump opcodes in the generator. JUMPBY is used for caches so it’s no help. Maybe we could have a SKIP_CACHE macro and the a jumpby is really a jump? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum
left a comment
There was a problem hiding this comment.
Also I don’t know how to identify the jump opcodes in the generator. JUMPBY is used for caches so it’s no help. Maybe we could have a SKIP_CACHE macro and the a jumpby is really a jump?
Yeah, disambiguating these through their macro names sounds fine.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum
commented
Jun 13, 2023
LG. Next steps? |
iritkatriel
commented
Jun 13, 2023
next is to rebase and enable the assertions for pseudo ops. If that worksI'll check the diff, maybe this PR is done then. |
iritkatriel
commented
Jun 13, 2023
Maybe we should make the parser or generator complain if you use |
gvanrossum
commented
Jun 13, 2023
How would it know? Maybe the original idea of looking for that sequence of tokens isn't so bad. Just don't fold it into |
iritkatriel
commented
Jun 13, 2023
I didn't like that |
gvanrossum
commented
Jun 13, 2023
I grepped and I see about a dozen places where frame->f_code is used to access something other than co_consts or co_names. So you'd need to review those. |
| [SEND] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG }, | ||
| [SEND_GEN] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG }, | ||
| [INSTRUMENTED_YIELD_VALUE] = { true, INSTR_FMT_IB, HAS_ARG_FLAG }, | ||
| [YIELD_VALUE] = { true, INSTR_FMT_IB, HAS_ARG_FLAG }, |
There was a problem hiding this comment.
I just noticed that this PR changed the format of YIELD_VALUE/INSTRUMENTED_YIELD_VALUE from INSTR_FMT_IX to INSTR_FMT_IB. This is because we added the assertion that makes the generator identify it has HAS_ARG. I guess the new value is correct?
There was a problem hiding this comment.
In opcode.py it is classified as having an arg (by being >= HAVE_ARGUMENT) so I think it's correct. The oparg is used elsewhere to indicate the stack depth.
This is not yet working for pseudo instructions. Should we add them to bytecodes.c in some form?
Also, I needed to add a silly assert to YIELD_VALUE (which is irregular) to make it look to the code generator like it uses oparg. The bytecode doesn't use the oparg but oparg is set to the exception stack depth, so the opcode needs to be considered as HAS_ARG because there are assertions that oparg is 0 for instructions without args.