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-125063: Move slice constant-folding to AST #126830

Closed
wants to merge 2 commits into from

Conversation

encukou
Copy link
Member

@encukou encukou commented Nov 14, 2024

Generate slice constants in AST, rather than in codegen.

This could allow more optimizations in earlier steps, albeit in practice it probably only helps rare cases (e.g. "abc"[:1] -> "a').

Tests for the final result were added in GH-125064.

@encukou encukou changed the title gh-125063: Turn slices of constants to constant slices in AST gh-125063: Turn slices with constants to constant slices in AST Nov 14, 2024
@encukou encukou changed the title gh-125063: Turn slices with constants to constant slices in AST gh-125063: Move slice constant-folding to AST Nov 14, 2024
self.assertInBytecode(code, 'LOAD_CONST', '\uffffx')
self.assertNotInBytecode(code,'BINARY_SUBSCR')
self.check_lnotab(code)

Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking this test doesn't belong in test_peepholer now that the optimization is not part of the peephole optimiser.

Where are the other const folding tests?

Copy link
Member

Choose a reason for hiding this comment

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

You're right, Irit.

This test should be re-written and placed in the test_ast/test_ast.py::ASTOptimiziationTests

@markshannon
Copy link
Member

I want to suggest the opposite: remove the AST optimizer altogether.

The flowgraph optimizer has more information and can do a better job.
In fact the AST optimizer prevents the flowgraph optimizer producing the best code by mis-optimizing in some cases.
For example, by doing premature constant folding, This:

   a, b = 1,2

is compiled to

  LOAD_CONST               0 ((1, 2))
  UNPACK_SEQUENCE          2
  STORE_FAST_STORE_FAST    1 (a, b)

but it should be (and would be but for the AST optimizer) compiled to this:

  LOAD_SMALL_INT           1
  LOAD_SMALL_INT           2
  STORE_FAST_STORE_FAST    1 (a, b)

There are a few changes that the AST optimizer does that the flowgraph optimizer doesn't, and they could all be easily add
ed to the flowgraph optimizer.

The AST optimizer converts UNARY_OP("-", CONST(1)) to CONST(-1) which needs to be done before code gen, but other than that everything else should be done in the flowgraph optimizer.

@markshannon
Copy link
Member

One more example that is probably more relevant to this PR.

  x[:1]

is currently compiled as:

  LOAD_FAST                0 (x)
  LOAD_CONST               0 (slice(None, 1, None))
  BINARY_SUBSCR

we might want to compile it as:

  LOAD_FAST                0 (x)
  LOAD_CONST               0 (None)
  LOAD_SMALL_INT           1
  BINARY_SLICE

This PR would prevent that.

@Eclips4
Copy link
Member

Eclips4 commented Nov 14, 2024

I concur with Mark. I would like to see the elimination of the ast optimizer, and I would be happy to do it.

@encukou
Copy link
Member Author

encukou commented Nov 15, 2024

OK, so I'll close this then :)

@encukou encukou closed this Nov 15, 2024
@Eclips4
Copy link
Member

Eclips4 commented Nov 15, 2024

OK, so I'll close this then :)

Thank you for your efforts, Petr!

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