Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-87092: create a 'jump target label' abstraction so that the compiler's codegen stage does not work directly with basic blocks #95398

Merged
merged 16 commits into from
Aug 4, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jul 28, 2022

This does two things:

(1) prepares the code for moving codegen from basicblock to proper labels (which are replaced by basicblock refs before the optimization stage).
(2) reduces boilerplate and tightens error checking in a few places.

…compiler's codegen stage does not work directly with basic blocks
@iritkatriel iritkatriel added interpreter-core (Objects, Python, Grammar, and Parser dirs) 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Jul 28, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 863f188 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 28, 2022
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

Could you rename SET_LABEL to something a bit more meaningful?

Logically we are emitting labels in the same way as we are emitting instructions.
However, we aren't really doing that physically.

I'd be happy with EMIT_LABEL or EMIT_LABEL_HERE, assuming the implementation will match the concept soon enough.
Or we could use USE as the old code was compiler_use_next_block.
Either USE_LABEL or USE_LABEL_HERE?

It's your call, but "set" has so many meanings as to be meaningless.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@iritkatriel
Copy link
Member Author

I have made the requested changes; please review again.

I also made a few more changes that make this refactor more complete - we now have both a target_label and a target (block) on the instruction. The label is set by the frontend and the block is calculated before optimisations. They have different types to make sure we know which one we are using when.

Eventually the i_target can move off the instruction to the basicblock (there is only one per BB). Once the label becomes an int we can use the oparg field for it, so the instruction won't need either of these two fields.

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 29, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 0b8075e 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 29, 2022
@iritkatriel
Copy link
Member Author

Buildbots are happy with this.

@iritkatriel
Copy link
Member Author

iritkatriel commented Aug 3, 2022

With the last change the front end's labels are int rather than basicblock*.. (reverted a couple of commits - test_venv hanged).

@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 3, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 5415722 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 3, 2022
@iritkatriel
Copy link
Member Author

I found the bug that caused the reverted commit to fail, but I think we should merge this PR as is and continue in another one.

This PR currently touches a lot of the code, but the changes are simple. The next step is a bit more sensitive so it's better to have it in a smaller PR.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants