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-120619: Tier 2 partial evaluator foundations #124910

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Oct 2, 2024

This PR sets up a tier 2 partial evaluation pass' foundations. It does the following:

  1. Introduce binding-time analysis of instructions and locals.
  2. Virtualized stack values (This was why we need a local slot now to indicate if the stack value is pointing to a virtualized instruction).
  3. A new abstract interpreter -- one for partial evaluation.
  4. Symbolic stack reification -- reconstructing the stack when we need to/hit a non-static instruction. This is done by telling the originating instruction to devirtualize itself.
  5. It removes most pure annotations -- we need to reimplement those in the partial evaluator for every one we introduce. Otherwise it will do arbitrary optimizations on them.

As a litmus test, it does simple dead store elimination by tracking locals. For example, the code x = x is now optimized to a nop.

This is different than the previous PR at #123652. The main difference is that we create the residual in-place. By "looking backwards" at the originating instruction and marking that as virtual/non-virtual. If you want a visualization of how this will work. See Mark's talk at EuroPython 2023 here https://youtu.be/dgrtgtT-UXM?t=1198.

Also up for discussion: the generator for the partial evaluator is exactly the same as the specializer. Should I just remove it?

Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

Great work, Ken!

Makefile.pre.in Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
@markshannon
Copy link
Member

markshannon commented Oct 3, 2024

I don't understand why any of bytecodes.c, optimizer_analysis.c, optimizer_bytecodes.c, optimizer_symbols.c, or the structs in pycore_optimizer.h need to change.
This is adding a new pass. There should ideally be no changes to the existing pass. If changes are necessary they should be few and small.

What does _static mean? Using "static" is very confusing unless it relates to the C "static" which describes scope.

Rather than removing the pure annotations, we should make sure that they are in the right places.
e.g..
LOAD_FAST is marked as pure, but it clearly has side effects.
BINARY_SUBSCR_STR_INT and BINARY_SUBSCR_TUPLE_INT are not marked as pure, but they are.

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Oct 3, 2024

I don't understand why any of bytecodes.c, optimizer_analysis.c, optimizer_bytecodes.c, optimizer_symbols.c, or the structs in pycore_optimizer.h need to change.

This is adding a new pass. There should ideally be no changes to the existing pass. If changes are necessary they should be few and small.

I reused the infra for the old pass to build the new one. Though I can separate them totally for separation of concerns. Let me know what you think.

What does _static mean? Using "static" is very confusing unless it relates to the C "static" which describes scope.

It means potentially static in the context of binding-time analysis. It's more liberal than pure.

@Fidget-Spinner
Copy link
Member Author

@markshannon I split it out into its own files, and own symbols, so it doesn't share the type information too. This simplifies the lattice quite a bit.

@erlend-aasland erlend-aasland removed their request for review October 14, 2024 08:07
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.

3 participants