FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-101799: implement PREP_RERAISE_STAR as an intrinsic function by iritkatriel · Pull Request #101800 · python/cpython · GitHub

/ cpython Public

gh-101799: implement PREP_RERAISE_STAR as an intrinsic function - #101800

Merged
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:prep-reraise
Feb 14, 2023
Merged

gh-101799: implement PREP_RERAISE_STAR as an intrinsic function#101800
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:prep-reraise

Conversation

iritkatriel commented Feb 10, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Quick comment: Why not add a CALL_INTRINSIC_2 opcode (assuming we expect more of these)? The variable stack effect based on comparison with an arbitrary constant feels a little fragile. Maybe it would feel less arbitrary if the constant was actually 128 (i.e., bit 7, 0x80).

Copy link
Copy Markdown
Member Author

Quick comment: Why not add a CALL_INTRINSIC_2 opcode (assuming we expect more of these)? The variable stack effect based on comparison with an arbitrary constant feels a little fragile. Maybe it would feel less arbitrary if the constant was actually 128 (i.e., bit 7, 0x80).

I considered this, the down side is we have two opcodes for non-perf-critical stuff. I don't have a strong opinion either way. @markshannon ?

Copy link
Copy Markdown
Member Author

Is FORMAT_VALUE performance critical? It could be replaced by a couple of intrinsic functions.

Copy link
Copy Markdown
Member

Is FORMAT_VALUE performance critical? It could be replaced by a couple of intrinsic functions.

Possibly. Most of it could become a helper function. This would lose the fast path for f"aaa{foo}zzz" if foo is exactly a Unicode string, but I'm not sure how important that really is.

Copy link
Copy Markdown
Member

Usually we want to do dispatching in the dispatch loop, not within instructions.
However, the point of the CALL_INTRINSIC instruction is to reduce the umber of instructions, so its less clear cut.

On balance, I still think we want two instructions. The extra test and branch is messy; two instructions would make for clearer code and we aren't that short of opcodes.

Copy link
Copy Markdown
Member

Regarding FORMAT_VALUE, I refer you to the venerable #6132 which split FORMAT_VALUE into CONVERT_VALUE, FORMAT_SIMPLE and FORMAT_WITH_SPEC.

CONVERT_VALUE could be replaced with CALL_INSTINSIC_1 and FORMAT_WITH_SPEC could be replaced with CALL_INSTINSIC_2. FORMAT_SIMPLE is simple enough, and probably common enough, that it merits its own instruction.

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL