gh-105481: remove regen-opcode. Generated _PyOpcode_Caches in regen-cases.#108367
gh-105481: remove regen-opcode. Generated _PyOpcode_Caches in regen-cases.#108367iritkatriel merged 5 commits intopython:mainfrom
Conversation
| if instr.cache_offset > 0: | ||
| self.out.emit(f'[{name}] = {instr.cache_offset},') | ||
| # Irregular case: | ||
| self.out.emit('[JUMP_BACKWARD] = 1,') |
There was a problem hiding this comment.
How should we deal with this case?
There was a problem hiding this comment.
Put JUMP_BACKWARD in a family of its own, and add the cache effect notation to its instr? Or is that a problem because it's also part of the JUMP pseudo?
If it's complicated I'm fine with leaving this special case.
There was a problem hiding this comment.
Alas, it looks like it's in a pseudo with JUMP_FORWARD and those are required to have the same size. Maybe that requirement is too strict?
There was a problem hiding this comment.
In an offline discussion with @markshannon we decided to add a family for JUMP_BACKWARD which would declare its cache size. This causes the generator to emit an assertion that the cache size declared for the family (1) is equal to that which is calculated from the instruction signature (0). If I add the cache to the signature, then I get an error about the pseudo-instructions for JUMP (JUMP_FORWARD and JUMP_BACKWARD) not having the same instruction format.
So there is something here that we need to resolve, and it's not a trivial change. I suggest we commit this PR (which is large enough) with the workaround on line 553, and resolve it in a separate PR.
There was a problem hiding this comment.
Ah, I didn't see your comment before I wrote this ^. Yes, we seem to agree.
| if instr.cache_offset > 0: | ||
| self.out.emit(f'[{name}] = {instr.cache_offset},') | ||
| # Irregular case: | ||
| self.out.emit('[JUMP_BACKWARD] = 1,') |
There was a problem hiding this comment.
Put JUMP_BACKWARD in a family of its own, and add the cache effect notation to its instr? Or is that a problem because it's also part of the JUMP pseudo?
If it's complicated I'm fine with leaving this special case.
Co-authored-by: Guido van Rossum <[email protected]>
|
There's a new commit after the PR has been approved. @gvanrossum: please review the changes made to this pull request. |
3 participants
Fixes #105481 .