gh-126835: Disable tuple folding in the AST optimizer

[tomasr8]

This disables tuple-folding in the AST optimizer which allows the flowgraph to optimize them instead.
Related comment: #126830 (comment)

I've done some local benchmarking with pyperf which showed some speedup for unpack_sequence but it'd be nice to have proper benchmarks for this :)

cc @Eclips4

• Issue: Remove the AST optimizer #126835

[markshannon]

Why have tests been removed from test_compile and test_peepholer?
Those test the code after the CFG optimizer has run, so should be unchanged.

The tests in test_opcache should not be removed, but should be changed to keep the unpacking operation by moving the tuple out of the loop.
Instead of:

for ...
    a, b = 1, 2

use

t = 1, 2
for ...
    a, b = t

[Eclips4]

Why have tests been removed from test_compile and test_peepholer? Those test the code after the CFG optimizer has run, so should be unchanged.

test_compile was changed because the test was fundamentally wrong. The name of the test suggests that it tests constant merging, but actually, all the work was done by AST optimizer. Constant merging has never had any relation to this test. In particular, it tested that co_consts for lambda: (257,) would look like a ((257,),) instead of (257, (257,)). Do you think it's worth fixing?

test_peepholer only changed where it tries to fold set into frozenset where one of the elements is constant tuple. Since folding of constant tuples are handled by the CFG, set folding during the AST optimization can't work for such case. We could add folding for sets (not in all cases, only with in operator and where set is a target for for loop) in the follow-up PR.

The tests in test_opcache should not be removed, but should be changed to keep the unpacking operation by moving the tuple out of the loop.

Thank you, for some reason I forgot about non-constant case...

[markshannon]

Constant merging has never had any relation to this test. In particular, it tested that co_consts for lambda: (257,) would look like a ((257,),) instead of (257, (257,)). Do you think it's worth fixing?

No. You can remove that test. Try to keep any tests that test behavior, but any that just test artifacts (like the test for lambda: (257,) ) can be removed.

We could add folding for sets (not in all cases, only with in operator and where set is a target for for loop) in the follow-up PR.

Yes, leave that for another PR.

As an aside:
From a quick experiment I did a while ago, I think the only folding that needs to be done before the CFG optimizer is for negative numbers, converting -(1) into -1. I think that can be done in the codegen pass.

[Eclips4]

As an aside: From a quick experiment I did a while ago, I think the only folding that needs to be done before the CFG optimizer is for negative numbers, converting -(1) into -1. I think that can be done in the codegen pass.

I think we could do that in the follow-up PR.
Though, I'm not an parser expert, but when I understood that parser generates ast.UnaryOp node instead of ast.Constant for simple constants like -1, I was confused.
@pablogsal as parser expert, is it hard to do this in the parser?

@markshannon do you think it's ok to merge this PR? This PR only removes parts of test_ast which are basically tests AST optimizations, and incorrect test in test_compile. Any other changes are simply commenting some parts of tests, which we can uncomment later when other optimizations will take place in CFG.

[pablogsal]

@pablogsal as parser expert, is it hard to do this in the parser?

This normally should not be done in the parser as the general idea is that the parser gives the most pure AST to the next step in the pipeline and every modification and optimisations happens there. There are many reasons for this but the classical one is to not make the life of formatters and similar tools hard and not to make unparsing harder.

[Eclips4]

There are many reasons for this but the classical one is to not make the life of formatters and similar tools easy

I guess you mean to use the word "hard" here. Otherwise, do the parser team make someone's life harder on purpose? 🤣

[pablogsal]

I guess you mean to use the word "hard" here.

Haha, indeed! 😆

Otherwise, do the parser team make someone's life harder on purpose? 🤣

Maybe just our own life 😉

[Eclips4]

FYI, I plan to merge this tomorrow, as this PR is quite small and I don't see any points here which need further discussion. Mark's comments have already been addressed.

[iritkatriel]

I think this PR will need documentation because ast.parse no longer optimises this, and it's part of the stdlib API. If the optimisations in final bytecode changed then it needs to be mentioned in WhatsNew.

[iritkatriel]

It's not disabled in the AST optimiser, it is removed. The intention here is not clear.
Are we giving up on this case? If so, we should remove the test rather than comment it away.
If we want to implement this in another PR, then we need to create an issue rather than leave a comment.

[iritkatriel]

Sorry, I now read the whole discussion. My preference would be a single PR that moves const folding from the AST to the compiler, rather than two PRs that leave things in an intermediate broken state.

As you said, this PR is not very large. If it was I would be more inclined to split it.

In any case, this should be mentioned in WhatsNew. If someone upgrades to a new version and something changes for them, they should be able to find a hint in the release notes what could have caused it.

[Eclips4]

I'm agreed.
...And unfortunately, that's where we encounter the intersection of optimizations from the codegen phase and the CFG (in fact, part of this optimization handles tuples!).

Codegen tries to optimize this

LOAD_CONST 0 'a' 
LOAD_CONST 1 'b'
LOAD_CONST 2 'c'
BUILD_SET 3

to this

BUILD_SET 0
LOAD_CONST frozenset({'a', 'b', 'c'})
SET_UPDATE 1

Now I'm not sure what we should do about that.

[iritkatriel]

How is this interfering with const tuple folding?

[iritkatriel]

Ah, I see. You want this to be after tuple folding. Right? Maybe this optimization can move from codegen to flowgraph?

[Eclips4]

I think that, yes, we could move this optimization from codegen to the flowgraph. Though I'm not sure if we should do this in this PR. What do you think?

[iritkatriel]

Up to you. You could perhaps do that first in a separate PR and then rebase this and finish it cleanly.

[Eclips4]

Up to you. You could perhaps do that first in a separate PR and then rebase this and finish it cleanly.

Agreed. Doing this in a separate PR looks cleaner to me. PR is here: #129426

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.