Skip to content

gh-92782: unify the style of CFG traversal algorithms in the compiler #92784

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

Merged
merged 7 commits into from
May 17, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented May 13, 2022

This makes the traversals more consistent:

  • reduces repetition of stack allocation code
  • makes them all use PyMem_Malloc (currently there are two that use PyObject_Malloc)
  • always counts the blocks (a.a_nblocks can be stale) and uses the b_next list, which excludes unreachable (empty) blocks
  • always initialises b_visited before a traversal
  • mark_reachable uses b_visited
  • eliminate_empty_basic_blocks fixes except targets like jump targets (see discussion below)

@iritkatriel iritkatriel changed the title hg-92782: unify the style of CFG traversal algorithms in the compiler gh-92782: unify the style of CFG traversal algorithms in the compiler May 13, 2022
@iritkatriel
Copy link
Member Author

The test failure seems to imply that in a subprocess we can end up calling mark_reachable multiple times on the same code. Is this expected?

@iritkatriel
Copy link
Member Author

I can't reproduce that test failure on my machine, so I'm reverting that particular change.

Copy link
Member

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

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

These changes look correct to me, though I have a slightly-off-topic question:

When traversing via b = b->b_next, how is it ensured that exception handlers stay in that linked list? Can empty exception handlers exist? If so, can they get removed by eliminate_empty_basic_blocks and then leave connected basicblocks dangling?

@iritkatriel
Copy link
Member Author

When traversing via b = b->b_next, how is it ensured that exception handlers stay in that linked list? Can empty exception handlers exist? If so, can they get removed by eliminate_empty_basic_blocks and then leave connected basicblocks dangling?

I think you're right that eliminate_empty_basic_blocks should fix targets of block_push instructions in the same way that it does for jumps.

I think we're getting away with this at the moment the first block of the handler always has a PUSH_EXC_INFO instruction, so it's never empty.

@iritkatriel
Copy link
Member Author

I believe I found out why making mark_reachable use b_visited failed.

It is currently possible to get a conditional jump to the next block. So b_next == b_target, and my assertion assumed this is not the case.

I've updated the assertion and put that change back in this PR. We'll see what the buildbots think.

@iritkatriel
Copy link
Member Author

@markshannon I think this is ready to be merged. Last chance to object review.

@markshannon
Copy link
Member

No objections. LGTM.

@iritkatriel iritkatriel merged commit 8781a04 into python:main May 17, 2022
@iritkatriel iritkatriel deleted the traversal branch May 20, 2022 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants