Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-105481: remove regen-opcode. Generated _PyOpcode_Caches in regen-cases.#108367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 .