Skip to content

Tier 2 cleanups and tweaks #115534

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

Merged
merged 20 commits into from
Feb 20, 2024
Merged

Tier 2 cleanups and tweaks #115534

merged 20 commits into from
Feb 20, 2024

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 15, 2024

  • Rename _testinternalcapi.get_{uop,counter}_optimizer to new_*_optimizer
  • Use _PyUOpName() instead of _PyOpcode_uop_name[]
  • Add target to executor iterator items -- list(ex) now returns (opcode, oparg, target, operand) quadruples
  • Add executor methods get_opcode() and get_oparg() to get vmdata.opcode, vmdata.oparg
  • Define a helper for printing uops, and unify various places where they are printed
  • Add a hack to summarize_stats.py to fix legacy uop names (e.g. POP_TOP -> _POP_TOP)

@mdboom
Copy link
Contributor

mdboom commented Feb 15, 2024

Regarding _PyUOpName, specialize.c has something different. I guess the names < 256 are shared with the Tier 1 instructions, but I don't know the history there. Maybe we use _PyUOpName here, too?

for (int i = 0; i < 512; i++) {
        if (i < 256) {
            names = _PyOpcode_OpName;
        } else {
            names = _PyOpcode_uop_name;
        }
        if (stats->opcode[i].execution_count) {
            fprintf(out, "uops[%s].execution_count : %" PRIu64 "\n", names[i], stats->opcode[i].execution_count);
        }
        if (stats->opcode[i].miss) {
            fprintf(out, "uops[%s].specialization.miss : %" PRIu64 "\n", names[i], stats->opcode[i].miss);
        }
    }

@gvanrossum
Copy link
Member Author

Regarding _PyUOpName, specialize.c has something different. I guess the names < 256 are shared with the Tier 1 instructions, but I don't know the history there. Maybe we use _PyUOpName here, too?

I saw that, and didn't immediately know how to rewrite that. Looking again I now know what to do (I had misunderstood what that code does) and I will fix this.

@gvanrossum
Copy link
Member Author

gvanrossum commented Feb 15, 2024

I saw that, and didn't immediately know how to rewrite that. Looking again I now know what to do (I had misunderstood what that code does) and I will fix this.

Fixed. There's one change: when an uop is also a Tier-1 op, the previous version would print the Tier-1 name (without a leading underscore). The new version always prints the Tier-2 name. E.g.

> uops[POP_TOP].execution_count : 83
---
> uops[_POP_TOP].execution_count : 83

I believe the latter is more correct.

@mdboom
Copy link
Contributor

mdboom commented Feb 16, 2024

I believe the latter is more correct.

Yeah, it probably is. This will make the stats before and after this change no longer comparable. That's probably fine -- if we find ourselves needing to do that the update to summarize_stats.py should be straightforward.

@gvanrossum
Copy link
Member Author

This will make the stats before and after this change no longer comparable. That's probably fine -- if we find ourselves needing to do that the update to summarize_stats.py should be straightforward.

Oh, that's unfortunate. Didn't we just recently have a big stir about unexplained stats changes?

Regarding a possible fix, were you thinking of just changing uops[POP_TOP].execution_count into uops[_POP_TOP].execution_count (etc.) in load_raw_data(), just before the line stats[key.strip()] += int(value)? I'm tempted to do that.

@gvanrossum gvanrossum marked this pull request as ready for review February 16, 2024 01:11
@gvanrossum
Copy link
Member Author

I foresee some merge conflicts with gh-114142. I'd rather see that merged first.

@mdboom
Copy link
Contributor

mdboom commented Feb 16, 2024

Regarding a possible fix, were you thinking of just changing uops[POP_TOP].execution_count into uops[_POP_TOP].execution_count (etc.) in load_raw_data(), just before the line stats[key.strip()] += int(value)? I'm tempted to do that.

Yes, exactly. Just something to normalize all the uops to have a _ prefix if the data doesn't already have it.

@Fidget-Spinner
Copy link
Member

Small merge conflict with #115550.

@gvanrossum
Copy link
Member Author

@markshannon Could you review this? I intend to merge after your side exits PR lands.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

One suggestion, otherwise LGTM

@gvanrossum gvanrossum enabled auto-merge (squash) February 20, 2024 19:26
@gvanrossum gvanrossum merged commit 142502e into python:main Feb 20, 2024
@gvanrossum gvanrossum deleted the opt-tweaks branch February 20, 2024 21:22
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
* Rename `_testinternalcapi.get_{uop,counter}_optimizer` to `new_*_optimizer`
* Use `_PyUOpName()` instead of` _PyOpcode_uop_name[]`
* Add `target` to executor iterator items -- `list(ex)` now returns `(opcode, oparg, target, operand)` quadruples
* Add executor methods `get_opcode()` and `get_oparg()` to get `vmdata.opcode`, `vmdata.oparg`
* Define a helper for printing uops, and unify various places where they are printed
* Add a hack to summarize_stats.py to fix legacy uop names (e.g. `POP_TOP` -> `_POP_TOP`)
* Define helpers in `test_opt.py` for accessing the set or list of opnames of an executor
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
* Rename `_testinternalcapi.get_{uop,counter}_optimizer` to `new_*_optimizer`
* Use `_PyUOpName()` instead of` _PyOpcode_uop_name[]`
* Add `target` to executor iterator items -- `list(ex)` now returns `(opcode, oparg, target, operand)` quadruples
* Add executor methods `get_opcode()` and `get_oparg()` to get `vmdata.opcode`, `vmdata.oparg`
* Define a helper for printing uops, and unify various places where they are printed
* Add a hack to summarize_stats.py to fix legacy uop names (e.g. `POP_TOP` -> `_POP_TOP`)
* Define helpers in `test_opt.py` for accessing the set or list of opnames of an executor
LukasWoodtli pushed a commit to LukasWoodtli/cpython that referenced this pull request Jan 22, 2025
* Rename `_testinternalcapi.get_{uop,counter}_optimizer` to `new_*_optimizer`
* Use `_PyUOpName()` instead of` _PyOpcode_uop_name[]`
* Add `target` to executor iterator items -- `list(ex)` now returns `(opcode, oparg, target, operand)` quadruples
* Add executor methods `get_opcode()` and `get_oparg()` to get `vmdata.opcode`, `vmdata.oparg`
* Define a helper for printing uops, and unify various places where they are printed
* Add a hack to summarize_stats.py to fix legacy uop names (e.g. `POP_TOP` -> `_POP_TOP`)
* Define helpers in `test_opt.py` for accessing the set or list of opnames of an executor
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