GeezerVison

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.

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

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.

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.

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

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

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.

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

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!