Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
Comments
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How should we deal with this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Put JUMP_BACKWARD in a family of its own, and add the cache effect notation to itsinstr? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In an offline discussion with@markshannon we decided to add a family forJUMP_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 forJUMP (JUMP_FORWARD andJUMP_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, I didn't see your comment before I wrote this ^. Yes, we seem to agree.
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Put JUMP_BACKWARD in a family of its own, and add the cache effect notation to itsinstr? 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.
bedevere-bot commentedAug 23, 2023
There's a new commit after the PR has been approved. @gvanrossum: please review the changes made to this pull request. |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#105481 .