GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-87092: compiler's CFG construction moved to after codegen stage by iritkatriel · Pull Request #102320 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Feb 28, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

After we merge this I will make a PR to move codegen to a separate file.

Skipping news - we will have one entry for the entire refactor once it's done.

  • Issue: Split compiler into code-gen, optimizer and assembler. #87092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
iritkatriel added 14 commits February 28, 2023 11:35
@iritkatriel
compiler uses instruction stream to create codeobjects
43df9b3
@iritkatriel
remove unused code
2b43f88
@iritkatriel
use newg all the way
52a1d1d
@iritkatriel
remove double processing
9b737f1
@iritkatriel
split cfg_builder_use_label/cfg_builder_addop to separate versions fo…
083b8b1
…r codegen and cfg_builder
@iritkatriel
codegen functions take an instr_stream
a56bafe
@iritkatriel
free isntr_stream. Attach it to compiluer_unit (not cfg_builder)
8be1b28
@iritkatriel
move things around
741f773
@iritkatriel
labels generated by instr_stream instead of cfg_builder
fb07525
@iritkatriel
add implicit RETURN NONE to the stream
1b06bf8
@iritkatriel
remove u_cfg_builder
142a7f1
@iritkatriel
init the cfg_builder in instr_stream_to_cfg
f9f8443
@iritkatriel
stream->sequence
f6fcde9
@iritkatriel
instr->cfg_instr, codegen_instr->instr
84f5a9d
@iritkatriel
iritkatriel requested a review from markshannon as a code owner February 28, 2023 11:45
@bedevere-bot bedevere-bot added the awaiting core review label Feb 28, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 28, 2023
Split compiler into code-gen, optimizer and assembler. #87092
Closed
@iritkatriel iritkatriel added the skip news label Feb 28, 2023
iritkatriel added 2 commits February 28, 2023 15:29
@iritkatriel
make sure we always emit something to jump to at the end
cd0225d
@iritkatriel
INSTR_STREAM --> INSTR_SEQUENCE
b8f4acf
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 28, 2023
@bedevere-bot

bedevere-bot commented Feb 28, 2023

Copy link
Copy Markdown

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 28, 2023
iritkatriel and others added 2 commits March 1, 2023 18:15
@iritkatriel
free the cfg_builder in the test harness
258f142
@iritkatriel
Merge branch 'main' into instruction-stream
e5653ee
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 1, 2023
@bedevere-bot

bedevere-bot commented Mar 1, 2023

Copy link
Copy Markdown

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 1, 2023
@iritkatriel
Merge branch 'main' into instruction-stream
788da74
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 2, 2023
@bedevere-bot

bedevere-bot commented Mar 2, 2023

Copy link
Copy Markdown

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 2, 2023
@iritkatriel

iritkatriel commented Mar 2, 2023

Copy link
Copy Markdown
Member Author

The test failures are a couple of timeouts plus a couple of errors that are also showing up on other branches.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Mar 2, 2023

Copy link
Copy Markdown
Member

Do we really want another instruction struct and another instruction sequence struct?
A basic block, extended block and general instruction sequence, are much the same. The main differences being where jumps are allowed and how they are resolved.
The allowed use and meaning of branches should be clear from the context.
If you want stronger typing in C, just wrap the base instruction sequence:

typedef struct InstructionSequence InstructionSequence;
typedef struct {
    InstructionSequence instructions;
} InstructionList;

typedef struct {
    InstructionSequence instructions;
} BasicBlock;

The reason I'd like a single InstructionSequence, is that we can stick an object header in front of it and access it from Python.
Having instruction sequences be Python objects would make introspection and debugging so much nicer.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Mar 2, 2023

Copy link
Copy Markdown
Member Author

We can have an InstructionSequence type which is reused in basic blocks, but basic blocks have a lot more fields that we don't need for codegen, and they don't need a label map because they should have pointers to the target blocks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Mar 2, 2023

Copy link
Copy Markdown
Member

We can have an InstructionSequence type which is reused in basic blocks

That makes sense.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Mar 2, 2023

Copy link
Copy Markdown
Member Author

We can have an InstructionSequence type which is reused in basic blocks

That makes sense.

I'm not sure.

The instruction in a basicblock is

struct cfg_instr {
    int i_opcode;
    int i_oparg;
    location i_loc;
    /* The following fields should not be set by the front-end: */
    struct basicblock_ *i_target; /* target block (if jump instruction) */
    struct basicblock_ *i_except; /* target block when exception is raised */
};

We can replace the first 3 fields by an instruction struct (this makes sense, I only avoided it to keep the diff smaller). But we would still need the target pointers, or to create a parallel data structure in the basic block for the pointers (and keep it in sync with the instruction sequence when we insert or delete instructions inside a block). It would be simpler to write a function that translates a CFG to something that can be exposed to python.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Mar 2, 2023

Copy link
Copy Markdown
Member Author

In any case, I don't think we should refactor the CFG data structures in the same PR. It will be another huge change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 2 commits March 2, 2023 19:08
@iritkatriel
#ifndef NDEBUG around cfg_builder_check
fd7f2b3
@iritkatriel
create shared helper function for resizing arrays
57fbe36
@iritkatriel

iritkatriel commented Mar 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

I've created a function for resizing the doubling arrays, so this logic is not repeated 3 times now.

To share the instruction type between codegen and cfg, we could do:

struct cfg_instr {
    instruction i_instr;
    struct basicblock_ *i_target; /* target block (if jump instruction) */
    struct basicblock_ *i_except; /* target block when exception is raised */
};

instead of the current:

struct cfg_instr {
     int i_opcode;
     int i_oparg;
     location i_loc;
    struct basicblock_ *i_target; /* target block (if jump instruction) */
    struct basicblock_ *i_except; /* target block when exception is raised */
};

Do you think we should do it in this PR? It would make the diff quit a lot messier.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel and others added 2 commits March 3, 2023 16:36
@iritkatriel
remove obsolete comment
dc9f1f5
@iritkatriel
Merge branch 'main' into instruction-stream
f20e480
markshannon
markshannon reviewed Mar 7, 2023
View reviewed changes

@markshannon markshannon left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

A couple of questions, otherwise looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 3 commits March 7, 2023 12:08
@iritkatriel
address code review
fa1c66a
@iritkatriel
tweak comments
3528176
@iritkatriel
index -> idx to avoid github's hilighting
fb13e36
@iritkatriel

iritkatriel commented Mar 7, 2023

Copy link
Copy Markdown
Member Author

A couple of questions, otherwise looks good.

Thank you. I'll merge then.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
iritkatriel merged commit 54060ae into python:main Mar 7, 2023
@bedevere-bot bedevere-bot removed the awaiting core review label Mar 7, 2023
carljm added a commit to carljm/cpython that referenced this pull request Mar 7, 2023
@carljm
Merge branch 'main' into nodeaddict
e391218
* main:
  pythongh-102493: fix normalization in PyErr_SetObject (python#102502)
  pythongh-87092: compiler's CFG construction moved to after codegen stage (python#102320)
  pythongh-95913: Consolidate build requirements changes in 3.11 WhatsNew (pythonGH-98781)
  Remove redundant `_ensure_future` in favor of `ensure_future` in `asyncio` (python#102398)
  pythongh-95913: Edit Faster CPython section in 3.11 WhatsNew (pythonGH-98429)
  pythongh-90110: Fix the c-analyzer Tool (python#102483)
  pythongh-101759: Update macOS installer SQLite 3.40.1 checksum (pythongh-102485)
  Remove unused import of `warnings` from `unittest.loader` (python#102479)
  Add gettext support to tools/extensions/c_annotations.py (python#101989)
carljm added a commit to carljm/cpython that referenced this pull request Mar 8, 2023
@carljm
Merge branch 'main' into inlinecomp2
1c019a7
* main:
  pythongh-102493: fix normalization in PyErr_SetObject (python#102502)
  pythongh-87092: compiler's CFG construction moved to after codegen stage (python#102320)
  pythongh-95913: Consolidate build requirements changes in 3.11 WhatsNew (pythonGH-98781)
  Remove redundant `_ensure_future` in favor of `ensure_future` in `asyncio` (python#102398)
  pythongh-95913: Edit Faster CPython section in 3.11 WhatsNew (pythonGH-98429)
  pythongh-90110: Fix the c-analyzer Tool (python#102483)
  pythongh-101759: Update macOS installer SQLite 3.40.1 checksum (pythongh-102485)
  Remove unused import of `warnings` from `unittest.loader` (python#102479)
  Add gettext support to tools/extensions/c_annotations.py (python#101989)
@iritkatriel
iritkatriel deleted the instruction-stream branch April 3, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@iritkatriel @bedevere-bot @markshannon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.