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

bpo-44626: Merge basic blocks earlier to enable better handling of exit blocks without line numbers. #27138

Merged
merged 2 commits into from
Jul 15, 2021

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jul 14, 2021

Improve propagation of line numbers to artificial blocks.
This is an incomplete, but hopefully good enough, solution.
The proper solution requires more extensive CFG modification, which I don't think the current compiler design can support.

One day, I will implement https://bugs.python.org/issue42926 🙂

https://bugs.python.org/issue44626

@markshannon markshannon changed the title Merge basic blocks earlier to enable better handling of exit blocks without line numbers. bpo-44626: Merge basic blocks earlier to enable better handling of exit blocks without line numbers. Jul 14, 2021
}
}
break;
case FOR_ITER:
Copy link
Member

Choose a reason for hiding this comment

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

You don't want to call extend_block for the default case?

Copy link
Member Author

Choose a reason for hiding this comment

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

We've already extended the blocks. I don't think there is anything to be gained by doing it again.

@@ -7744,6 +7748,12 @@ assemble(struct compiler *c, int addNone)
}
}

for (basicblock *b = c->u->u_blocks; b != NULL; b = b->b_list) {
Copy link
Member

Choose a reason for hiding this comment

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

I am confused on why this works. Aren't the blocks supposed to be topologically linked already?

Copy link
Member Author

Choose a reason for hiding this comment

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

We haven't computed the entry block yet, so we use b_list.

It also happens that using the backwards order gives better results.
Jumps to exits tend to be forward, so going backwards will be able to combine chains of jumps that going forwards would not.

I'm not making any claims that it is complete, it may well miss some blocks, but it should be good enough.

@pablogsal pablogsal added the needs backport to 3.10 only security fixes label Jul 14, 2021
@pablogsal pablogsal merged commit a86f7da into python:main Jul 15, 2021
@miss-islington
Copy link
Contributor

Thanks @markshannon for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Sorry @markshannon and @pablogsal, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker a86f7dae0acf918d54086cb85e5a0b0bedeedce7 3.10

@pablogsal
Copy link
Member

@markshannon Can you do the manual backport?

markshannon added a commit to markshannon/cpython that referenced this pull request Jul 16, 2021
…it blocks without line numbers (pythonGH-27138)

(cherry picked from commit a86f7da)
pablogsal pushed a commit that referenced this pull request Jul 16, 2021
…it blocks without line numbers (GH-27138) (GH-27182)

(cherry picked from commit a86f7da)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants