gh-131798: JIT: Optimize _POP_CALL_TWO_LOAD_CONST_INLINE_BORROW

[tomasr8]

Optimize _POP_CALL_TWO_LOAD_CONST_INLINE_BORROW

This requires adding some additional ops:

• _POP_CALL_ONE_LOAD_CONST_INLINE_BORROW
• _POP_CALL_LOAD_CONST_INLINE_BORROW
• _POP_THREE
• _POP_TWO

We can now optimize:

_LOAD_CONST_INLINE_BORROW_LOAD_CONST_INLINE_BORROW_POP_CALL_TWO_LOAD_CONST_INLINE_BORROW

into just

_POP_CALL_LOAD_CONST_INLINE_BORROW

i.e. we save two stack pushes and two stack pops.

• Issue: Better uop coverage in the JIT optimizer #131798

[Fidget-Spinner]

Can you remove the pure annotations from these please? They don't get optimized to anything so there's not really a use for them.

[Fidget-Spinner]

Thanks for the PR. I think @brandtbucher brought this up before, but I think we should be making the uop buffer add-to rather than in-place. What I mean is that instead of adding a pop variant for every optimization we want to do:

... _POP_X_LOAD_CONST_INLINE_BORROW 

We should add:

... _POP_TOP _POP_TOP ... _LOAD_CONST_INLINE_BORROW 

This way:

1. We don't have to add a new special pop uop for every single op. This saves on mental overhead, because a special pop variant of the uop for every uop we want to optimize is really tough to reason about.
2. In the optimizer, we can just generalize it to POP_TOP X times, which is easier to write code to optimize for, rather than add a new entry to remove_unneeded_uops. As you probably noticed already, it's really tedious to do this.

This would require allowing us to add to the uop buffer, rather than overwriting it in-place. It might be a bigger change, but if you're up for it, it would greatly simplify all future optimizations, and I'd be grateful for someone cleaning that part up. It would best be a separate PR.

[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.

[tomasr8]

Thanks for the PR. I think @brandtbucher brought this up before, but I think we should be making the uop buffer add-to rather than in-place.

We actually discussed this w/ Brandt yesterday and I totally agree with that. Brandt also mentioned that it is ok to add this for now but I don't mind closing this and focusing on changing the uop buffer right away.

Note that Brandt greatly simplified remove_unneeded_uops yesterday in #134373. Once I rebase this PR, it should end up being much simpler.

cc @brandtbucher

[brandtbucher]

Yeah, 90% of the reason why I felt the urgency to move away from in-place modification was because the logic in the pass that removes matching pushes and pops was getting completely out-of-hand. That's not a concern anymore, so I think we can keep the momentum going on the current in-place optimizer work for the time being.

[Fidget-Spinner]

Yeah, 90% of the reason why I felt the urgency to move away from in-place modification was because the logic in the pass that removes matching pushes and pops was getting completely out-of-hand. That's not a concern anymore, so I think we can keep the momentum going on the current in-place optimizer work for the time being.

Ok. Can we please make sure to move away from the in-place optimizer after this PR for any new pop tops load const inline X instructions? The pushes and pops are no longer an issue, but the number of new uops variants we're adding is getting out of hand. I have no clue what some of these uops do unless I specifically look up their emission in the optimizer. We don't need this many uops and fewer uops is generally a better thing (less maintainer burden, less human error ...). If we can generalize to just a few replicate(7) POP_TOP_X instructions, that would be awesome.

[brandtbucher]

Yep, I agree. It also allows us to specialize individual pops for specific known types, immortal values, non-escaping pops, etc. After the sprint ends tomorrow, we can start with larger cleanup efforts (which I agree we need, in the tracer too).

[Fidget-Spinner]

Great! Feel free to dismiss my review then in the GH UI.

[brandtbucher]

When I get back, I'll start working on making the optimizer operate on a copy of the stream. This is good for now though. Really cool to see entire calls start disappearing.

[brandtbucher]

Looks great. @tomasr8: natural follow-ups would be support for tuples of types in isinstance calls, as well as extending this optimization to the other calls we specialize (and seeing if we should possibly specialize more of them).

[tomasr8]

Looks great. @tomasr8: natural follow-ups would be support for tuples of types in isinstance calls, as well as extending this optimization to the other calls we specialize (and seeing if we should possibly specialize more of them).

on it!