gh-106290: Fix edge cases around uops by gvanrossum · Pull Request #106319 · python/cpython · GitHub
Skip to content

gh-106290: Fix edge cases around uops - #106319

Merged
gvanrossum merged 13 commits into
python:mainfrom
gvanrossum:tweak-uops
Jul 3, 2023
Merged

gh-106290: Fix edge cases around uops#106319
gvanrossum merged 13 commits into
python:mainfrom
gvanrossum:tweak-uops

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Jul 1, 2023

Copy link
Copy Markdown
Member

A bit of an omnibus PR, alas (let me know if you want it split up). This does roughly the following (though not in that order):

- Longer traces
- Explain reason for ending a trace
- Support STORE_FAST_LOAD_FAST, STORE_FAST_STORE_FAST
- Add deps on pycore_uops.h
This gives us LOAD_FAST_CHECK, LOAD_CLOSURE, and DELETE_FAST.
This doesn't work right yet, alas.
If an error occurs in the first uop, the lineno is -1.
The trick is that EXIT_TRACE must decrement prev_instr.
@gvanrossum

Copy link
Copy Markdown
Member Author

@gvanrossum

Copy link
Copy Markdown
Member Author

This (with uops on by default) benchmarks 2% slower than main (with uops off by default).

I will now revert the final (temporary) commit that turns on uops by default and mark it as ready for review.

(I just did that to be able to benchmark it and run the tests.)

This reverts commit 5b85a49.
@gvanrossum
gvanrossum marked this pull request as ready for review July 2, 2023 03:16
@gvanrossum
gvanrossum requested a review from markshannon as a code owner July 2, 2023 03:16
@gvanrossum

Copy link
Copy Markdown
Member Author

(Also, the tests that fail with uops on by default are largely expected. There's one refleak -- maybe I should ask Kumar to review it again. :-)

@gvanrossum

Copy link
Copy Markdown
Member Author

Maybe the leak is the optimizer created and installed initially.

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code itself looks good, haven't tried running it myself or anything. Just one note:

Comment thread Python/optimizer.c Outdated
return 0;

#undef ADD_TO_TRACE
#undef ADD_TO_TRACE_BASIC

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see where this is defined...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Turd from the last cleanup. I'll get rid of it, then land it.

Use it to `#undef DPRINTF` instead (which also depends on a variable in this scope).
@gvanrossum
gvanrossum enabled auto-merge (squash) July 3, 2023 19:29
@gvanrossum
gvanrossum merged commit 2028a4f into python:main Jul 3, 2023
@bedevere-bot

Copy link
Copy Markdown

@gvanrossum
gvanrossum deleted the tweak-uops branch July 5, 2023 15:45
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